Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build Issue with v6.4.1 min build file #187

Closed
JoeCoppola-HIA opened this issue Apr 15, 2022 · 14 comments
Closed

Build Issue with v6.4.1 min build file #187

JoeCoppola-HIA opened this issue Apr 15, 2022 · 14 comments

Comments

@JoeCoppola-HIA
Copy link
Contributor

JoeCoppola-HIA commented Apr 15, 2022

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.

@JoeCoppola-HIA JoeCoppola-HIA changed the title Build Issue with v6.4.2 min build file Build Issue with v6.4.1 min build file Apr 15, 2022
@swingingtom
Copy link
Collaborator

swingingtom commented Apr 15, 2022

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
Copy link

eviltik commented Apr 15, 2022

Hello

Yes there is a major webpack upgrade version 4 to 5.
See there (bottom of the page)
06402a4

What is your build command please ?

A problem with ES15 support ? it's an old issue but ..
angular/angular-cli#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
Copy link
Contributor Author

JoeCoppola-HIA commented Apr 15, 2022

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
Copy link
Collaborator

Unfortunately it always happen on fridays...

@JoeCoppola-HIA
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link

eviltik commented Apr 16, 2022

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

Consider this change:
13b25ec
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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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

@swingingtom
Copy link
Collaborator

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

@JoeCoppola-HIA
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants