Improvement on externalize React for React usages inside bundle codes

Question

Improvement on externalize React for React usages inside bundle codes

RichDom2185 opened this issue a year ago · 14 comments

RichDom2185 commented a year ago

Steps to Reproduce

With React DevTools extension installed, open Source Academy:
Launch any of the Unity Academy code examples
Click Run
Notice the change in the status reported by the extension

To investigate:

Is this message valid and reflective of what is actually happening?
Can this be fixed/optimised (if applicable)?

Answer 1 · 2023-05-13T10:02:53.000Z

Hello. I never heard about the "React Developer Tools", so here are my questions regarding this issue you mentioned.

Does this have any effects on the normal usage and functionality of Unity Academy and overall Source Academy Frontend? If so, what's the exact effects?
Since my module didn't use or import any new Node Modules to work besides those Node Modules that are originally in package.json before my module Unity Academy come in, and the "Unity Academy webgl frontend" itself is not related to React (it's JavaScript and WASM), and the only cause that I think can lead to this problem is that my module uses a separated React component (instead of Tab) to display Unity Academy window (the canvas for Unity to render and some control buttons), and this is NECESSARY for my module to work. So I'm not sure that what's this "React Developer Tools" are used for, and why it is related to my module.
If this is really caused by that separated React component, because if I try to modify that part, then it may lead to more potential problems and bugs since it's related to many things in my module including the Unity Academy frontend itself. So I'm not sure since everything is working fine for students to use now, dose this problem reported by a 3rd party tool really worthwhile to take the risk of potentially rising more new problems bugs to fix? Since I've tested my module with real functionality and usages for many times, and all bugs that I found in this process which affects usage have been fixed.
From the messages in the picture, it seems that this is related to the deployment process. So is it possible that this problem is actually coming from the module building system or deployment system which don't support well for adding new react components beside Tab? If so, I can't help much at this moment since currently I haven't study and not familiar with the build and development system.

Thanks

Answer 2 · 2023-05-13T10:47:04.000Z

Hi, thanks for the prompt reply! My replies below:

I never heard about the "React Developer Tools"...

React DevTools is a Chrome extension by Meta (developers of React) for developers to inspect/debug/profile websites made using React. You can do stuff like viewing the node structure/components of the page and their props, look at what part of the page is taking the most time to load, etc.

Does this have any effects on the normal usage and functionality of Unity Academy and overall Source Academy Frontend? If so, what's the exact effects?

There are no observable effects to a normal user using Source Academy. The point of concern is mainly that this extension shows this status change, which is unusual behaviour (other modules work fine). The focus of this GitHub issue is to investigate why this occurs and whether this may or may not be indicative of some underlying issue (e.g. performance problem, the way we execute modules, etc.).

... the only cause that I think can lead to this problem is that my module uses a separated React component (instead of Tab) to display Unity Academy window (the canvas for Unity to render and some control buttons), and this is NECESSARY for my module to work

Thanks, that could be a potential cause of the status change of the extension. I am not too familiar with the implementation of Unity Academy so I was not sure if there was any React component involved, but after your reply, it seems probable and worth investigating.

So I'm not sure since everything is working fine for students to use now, dose this problem reported by a 3rd party tool really worthwhile to take the risk of potentially rising more new problems bugs to fix?

That is fair, we don't have to fix it immediately, especially if you don't have the bandwidth to do so. We can always pick this up another time; at least we try to understand what is happening and the root cause of it first. Perhaps it is something so trivial the effort to fix it is just not worth it.

So is it possible that this problem is actually coming from the module building system or deployment system which don't support well for adding new react components beside Tab?

It is possible. I am not too familiar either. Perhaps @leeyi45 can chip in?

Thanks!

Answer 3 · 2023-05-13T11:00:21.000Z

Hello. Thanks for your explanation. From my view of my own module's code, my module creates a separated (covering the page) React component and inserting this new component directly in the current page to display Unity Academy, and this is related to the whole workflow and initialization of my module and Unity Academy Webgl frontend. Since this is working fine for functionality, and for now I haven't feel any observable efficiency problems (Unity Academy frontend has its internal optimizations such as auto-sleeping), so if you also think so, this problem may be not very urgent to fix.

Also, since my code for creating that new React component and inserting it into the page is common in usual React development (although this is not usual in SA modules that the fact is that other modules only has Tabs which are inserted by JS-Slang not module). So I think that possibility we should focus on how to change the module deployment and build system to solve this. Since if other modules in the future also requires uses their own React component (when Tab is not sufficient for functionality), fixing this issue by changing building system will make the module building system more flexible for these new requirements.

Unity Academy is using document.body.appendChild() and ReactDOM.render() to insert the separated React component under html page, and these two functions are usual in React and html development, so I think we should not forbid or change these two usages in modules, instead, we should try to make our module build system to fully support these two functions.

So personally I think this problem is more likely related to current module building system, and it is uncovered only by my module because unlike other modules, my module requires a new feature to use (the feature of having a separated React component besides Tab)

Answer 4 · 2023-05-13T11:10:04.000Z

So maybe we should close this issue and open a new issue regarding:

Defining new React components in bundle scripts and then using document.body.appendChild() and ReactDOM.render() in bundles to insert separated React component makes the module building system put a development version of React into bundles instead of production version

If this get fixed, in the future other modules can also use separated React component if needed without unusual warnings.

To fully make sure that this problem is related to "React components inside bundles scripts" instead of other things in Unity Academy module, you can try creating a totally empty SA module with an empty react component inside bundle script, then use document.body.appendChild() and ReactDOM.render() to insert that new component directly under the html document then use that DevTools to see if there is same errors (maybe this empty module need to be temporarily deploy to the production server to test this in production environment, so maybe you are more suitable to perform this test than me)

Answer 5 · 2023-05-13T11:47:52.000Z

We need to investigate whether that is really the cause of this issue. Either way, I don't think we should close this issue anytime soon. We'll probably just rename it and assign the relevant people once we finish investigating.

Answer 6 · 2023-05-13T12:08:52.000Z

@larrywang0701 does this issue occur with the other modules?

Module code is compiled directly to production, so even if you use the frontend in development mode, the loaded tabs will be React in production mode. This may be the reason for the indication

Maybe a way to test this is to change the script that builds the modules

The option is located under moduleUtils.ts

Answer 7 · 2023-05-13T12:21:41.000Z

@larrywang0701 does this issue occur with the other modules?

Module code is compiled directly to production, so even if you use the frontend in development mode, the loaded tabs will be React in production mode. This may be the reason for the indication

Maybe a way to test this is to change the script that builds the modules

The option is located under moduleUtils.ts

You need to ask @RichDom2185, he found out the issue.

If this only happen in my Unity Academy module, I think this may related to improper compilation behavior for the "React components" directed inside "bundle" scripts (NOT tabs). You mentioned that Tabs are directly compiled to production, but how about other React components inside bundles (apart from Tabs)?

To fully make sure that this problem is related to "React components inside bundles scripts" instead of other things in Unity Academy module, you can try creating a totally empty SA module with an empty react component inside bundle script, then use document.body.appendChild() and ReactDOM.render() to insert that new component directly under the html document then use that DevTools to see if there is same errors (maybe this empty module need to be temporarily deploy to the production server to test this in production environment, so maybe you are more suitable to perform this test than me)

Someone can perform this test to confirm the problem source. Better directly under sourceacademy.org (production environment frontend) instead of locally. So maybe the test module need to temporarily merge into production and then remove once test is finished.

Answer 8 · 2023-05-14T05:42:26.000Z

Apologies, I missed the part where you stated that the issue is specific to the unity academy.
@larrywang0701 You are correct I believe. For the tabs, React is marked as external, so it uses the React instance provided by the frontend. Most of the bundles don't have React components in their code, so I never thought to mark React as external for the bundles, which means that it is probably being bundled with the rest of the unity_academy bundle, which might explain the discrepancy.

A casual scroll through the built bundle for unity academy seems to indicate that this is the case. Is there a reason why this code has to be in the bundle?

Answer 9 · 2023-05-14T07:10:47.000Z

So personally instead of changing the code of Unity Academy, I think that we should try to make the module building system supports the feature of "adding custom React components inside bundles", since Tabs are not 100% sufficient under all cases (in the case of Unity Academy, Tabs are not sufficient), so in the future if other modules also require using their own React component inside bundle script, their functions won't be limited by the limitation of module building system. And also, since currently it's not very easy for bundles and tabs to exchange data (exchange data only by that context state variable), so if this limitation of "no React inside bundles" is removed, I think module programmers can also directly access and control tab components inside bundles, which will make their module more easy and flexible to make.

A casual scroll through the built bundle for unity academy seems to indicate that this is the case. Is there a reason why this code has to be in the bundle?

Tabs are too small in size for Unity Academy to display well (Unity Academy requires full-page display for best experience) and also, Unity Academy requires a canvas that always exists in the html document to work correctly, where Tabs will dismount and all html elements are destroyed when user switch away from Tabs

If it's necessary to make changes to Unity Academy's source, please tell me and I'll try move that separated React component from bundle to Tab source file, then test everything again to see whether it works well since such a change is a major change to my module, and I should do it by myself. But still, I think we should make the module building system support new requirements, not removing new requirements because of the limitations of building system.

Answer 10 · 2023-05-14T08:43:17.000Z

I won't be so fast as to recommend that bundles and tabs adopt the changes you've suggested, but I agree that there is a need for more customization with tabs and persistent module contexts.

If unity academy is working fine right now aside from the React development tools integration nothing needs to be changed.

I agree with supporting new requirements, just trying to minimize the amount of changes needed. I am unsure on how your custom component gets rendered, but I have a feeling that it won't work with React externalized. I don't think there's a (a straightforward, at least) way to get React imported into bundle code without bundling it with that code.

Answer 11 · 2023-05-14T08:49:59.000Z

I am unsure on how your custom component gets rendered

Inside bundle script:
If the component is not mounted (calling Unity Academy initialization for the first time), then Unity Academy creates a new <div> dom element called unity_container with document.createElement('div'), then append this element to document.body.
Then Unity Academy bundle code uses ReactDOM.render(...) to render the custom component under that unity_container element.

Answer 12 · 2023-05-14T10:09:07.000Z

I am unsure on how your custom component gets rendered

Inside bundle script: If the component is not mounted (calling Unity Academy initialization for the first time), then Unity Academy creates a new <div> dom element called unity_container with document.createElement('div'), then append this element to document.body. Then Unity Academy bundle code uses ReactDOM.render(...) to render the custom component under that unity_container element.

If you want you can try marking React as external for bundles, but like I said, I think by creating the element and rendering it this way you're using the instance of React bundled with your code.

Answer 13 · 2023-05-14T16:39:36.000Z

If you want you can try marking React as external for bundles

Just to clarify that in order to achieve this, we have to modify the overall module building system instead of Unity Academy module's source code right? Or it can be solved just by changing Unity Academy's code? If it's the second, I can try make changes to my own module. But currently I'm not familiar with module building system so maybe I can't help much with improving building system at this moment.

If unity academy is working fine right now aside from the React development tools integration nothing needs to be changed.

Yes, since everything in Unity Academy module is working as intended now except that DevTools warning, and if the answer to the question above is "changing module building system", maybe we can temporarily left everything just like this now, and if we figured out how to mark React as external for bundles, we can then upgrade our module building system in the future.

Answer 14 · 2023-05-14T17:57:01.000Z

I don't think there's a solution specific to unity academy, nor is there a solution that will work simply by changing the module build system without also modifying other parts like js-slang and the frontend.