felixmariotto / three-mesh-ui

⏹ Make VR user interfaces for Three.js
https://felixmariotto.github.io/three-mesh-ui/#basic_setup
MIT License
1.32k stars 141 forks source link

Build Issue with v6.4.1 min build file #187

Closed JoeCoppola-HIA closed 2 years ago

JoeCoppola-HIA commented 2 years ago

We are using Ionic version 5.4.16 in tandem with angular version 12.1.4. When building our app in a development environment, we have no issues with v6.4.1 as it directly uses the source files of the node package. However, when we build and deploy our app in a production environment, the minified build file for three mesh UI is used.

When we attempt to create a Block component, we get the following reference error:

ReferenceError: Must call super constructor in derived class before accessing 'this' or returning from derived constructor
    at new <anonymous> (three-mesh-ui.module.min.js:1:2853)
    at new re (three-mesh-ui.module.min.js:1:31134)

We have a lot of investigating to still perform, but I was hoping to see if there was any insight into what might be causing this. I noticed that the 6.4 version updates webpack, and was curious if there was a clash there possibly.

I will update this issue as we come across any new information related to this issue.

swingingtom commented 2 years ago

Hey @JoeCoppola-HIA , You did well to ask. There were indeed some changes that occured.

I added some samples and sandboxes to be sure of what I was releasing, but we can't anticipate everything

I've tested the minified builds you refer to on those sandboxes :
https://codesandbox.io/s/module-build-demo-minified-6-4-1-q6wybs?file=/index.html
https://codesandbox.io/s/npm-package-demo-6-4-1-2onzpo

But I can't notice anything. I've print the code behind Block constructor on npm-package-demo. It is called re same as your error.

Maybe one hint, how do you handle dependencies in your project? Especially threejs which is now externalized from three-mesh-ui bundle, so it needs to be loaded it prior to three-mesh-ui.

eviltik commented 2 years ago

Hello

Yes there is a major webpack upgrade version 4 to 5. See there (bottom of the page) https://github.com/felixmariotto/three-mesh-ui/commit/06402a488faa16b97e45896b710e1a799dabbf00

What is your build command please ?

A problem with ES15 support ? it's an old issue but .. https://github.com/angular/angular-cli/issues/7799

I didn't find interesting things webpack side.

Did you try/can you try a ionic update ? i can't find the version here https://github.com/ionic-team/ionic-framework/releases?page=7

JoeCoppola-HIA commented 2 years ago

Hello @eviltik and @swingingtom,

I am currently putting together a codesandbox example from the following minimal reproducing example that mirrors our app environment:

https://github.com/JoeCoppola-HIA/SimpleIonicThreeJSApp

This repo should be public and accessible to anyone to fork or pulldown and test. Maybe this would help spot anything we might be handling incorrectly.

To reproduce, simply npm install and run ionic serve --prod.

I will update this issue with the codesandbox when I have it running as well as try out the suggestions provided already. Thank you for the quick response.

Edit: Might have to install ionic as well

swingingtom commented 2 years ago

Unfortunately it always happen on fridays...

JoeCoppola-HIA commented 2 years ago

Ain't that the truth hahaha. Locking our version to 6.3.2 for the time being has our production issue resolved. But I would hate to miss out on future updates of Three Mesh UI because of this. We will figure it out, that I'm sure of it. Always appreciate your help.

JoeCoppola-HIA commented 2 years ago

Well it took me a little longer than I anticipated, but I was able to get an example of the behavior in a codesandbox example. The below link will load a node based sandbox that builds a simple production ionic app, creates a three js scene, places a blue cube and then tries to create a yellow three mesh ui block. On creation attempt it will throw the error I linked above. This will be alerted, and if you hit ok you will see the blue cube but not the yellow block.

https://codesandbox.io/s/broken-threemeshui-6-4-1-ionic-angular-15nti4?file=/src/app/home/home.page.ts

I also made a sandbox example of the exact same code, but this is building the ionic app without the prod flag, in this example, the error is not thrown:

https://codesandbox.io/s/threemeshui-6-4-1-ionic-angular-dev-build-h3c9zw?file=/src/app/home/home.page.ts

Hopefully this helps spot any issues with the way we handle dependencies here, or how Ionic Angular does. We attempted to update to the latest Angular locally, 13.3.3, as that version of build-angular uses the same version of Webpack that Three Mesh UI upgraded to. However we continue to get the same issue.

swingingtom commented 2 years ago

Hey @JoeCoppola-HIA, I was able to reproduce it from the github repository you shared.

Issue location

The resulted issue may not lay on ThreeMeshUI.Block itself. I've built the ionic serve --prod with both

import ThreeMeshUI from "three-mesh-ui/build/three-mesh-ui.module.min";
// which is currenlty the same of
// import ThreeMeshUI from "three-mesh-ui";

and

import ThreeMeshUI from "three-mesh-ui/build/three-mesh-ui.module";
// or even the un-built source file
// import ThreeMeshUI from "three-mesh-ui/src/three-mesh-ui";

The produced outputs of console.log(ThreeMeshUI.Block.toString() ); were exactly the same for both imports (except names that are optimized). So it can be on one of inherited constructors. Which I didn't analysed.

Please note, the later didn't suffer from the issue.

Workaround - Not Acceptable

I was able to get rid of this issue simply by setting projects.app.architect.configurations.production.optimization = false in angular.json file.
Even when forcing the minified module build in home.page.ts

import ThreeMeshUI from "three-mesh-ui/build/three-mesh-ui.module.min";

So troubles comes with combination of minified module build and angular optimisation. I didn't succeed to quickly find a list of angular optimisation options.

I did the same process with a webpack repository using terser optimisation, and didn't succeed to reproduce the issue

Workaround - Acceptable (IMO)

Previous v6.4.1, the module property defined in three-mesh-ui/package.json was three-mesh-ui/src/three-mesh-ui.js. v6.4.1 changed it to rely on three-mesh-ui/build/three-mesh-ui.module.min.js.

So you could get back your imports to :

// As the module build will still be optimized from angular side
import ThreeMeshUI from "three-mesh-ui/build/three-mesh-ui.module";

or

// Same results as < v6.4.1
import ThreeMeshUI from "three-mesh-ui/src/three-mesh-ui";

Next releases ??

Should we revert the module of three-mesh-ui/package.json to three-mesh-ui/src/three-mesh-ui.js ? (same as other libs such as three-mesh-bvh, etc...)

Should we downgrade the module of three-mesh-ui/package.json to three-mesh-ui/build/three-mesh-ui.module.js ? (same as threejs, etc...)

Both options won't remove the ability of module builds (unminified and minified).

eviltik commented 2 years ago

The build (prod) generate a class calling a constructor without super(). It's BoxComponent.

Consider this change: https://github.com/felixmariotto/three-mesh-ui/commit/13b25ec9555ce3b27f435c519ced7f7035ab2879 Search for "// @TODO: Replace .bind() when webpack update to allow class methods as fat arrow"

I can't make the test on my side, the idea should be to target ES5 in three-mesh-ui webpack config file using target: ['web', 'es5'] or target: ['es5'] to see if it's better or not (see https://webpack.js.org/migrate/5/)

My 2 cents

JoeCoppola-HIA commented 2 years ago

Thank you for looking into this. I am glad there are some workarounds that allow us to use the latest and greatest. Tomorrow I can test @eviltik's suggestion of building three mesh UI with the updated target in the config to see if that helps.

JoeCoppola-HIA commented 2 years ago

Modifying the browswerConfig in webpack.config.js to use target: ['web', 'es5'] results in the same super call error unfortunately.

However, updating all our import calls to import ThreeMeshUI from "three-mesh-ui/build/three-mesh-ui.module"; indeed resolved the issue, and still resulted in Angular optimizing the module in a minified fashion. This method does not use Types.d.ts, but that is acceptable as we weren't using them before due to their unfinished state. One day we would love to use them, but we aren't hindered by this for the time being.

As for what should be done in future releases, I ran a test to see if downgrading the module of three-mesh-ui/package.json to three-mesh-ui/build/three-mesh-ui.module.js would also fix our issue. It indeed fixes our Angular prod build, and if we opted to, would allow us to use Types.d.ts. While this works for us, as you said, we can't anticipate everything. But if a repo such as three js employs this strategy, with a user base as large as them, it might be proven more cross compiler safe?

swingingtom commented 2 years ago

I've update the package.json and pushed a new version v6.4.2
It comes with other improvements, meaning maybe more surprises... (see diff)

JoeCoppola-HIA commented 2 years ago

Thanks a bunch! We will work on getting this version into our app this week and evaluate any potential issues.

swingingtom commented 2 years ago

... as long as you don't break your production on next friday ;) 🪓 🔨

JoeCoppola-HIA commented 2 years ago

hahaha

@swingingtom just want to let you know that we upgraded to 6.4.2 in our dev environment. Running with the prod flag work now, and the overall updates I had to make to accommodate the main API changes were pretty straight forward and painless. Thank you a bunch again for this update. I feel more confident about being ready for 7.x.x.