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

Package.json: Added "sideEffects": false #21313

Merged
merged 1 commit into from
Feb 20, 2021
Merged

Package.json: Added "sideEffects": false #21313

merged 1 commit into from
Feb 20, 2021

Conversation

mrdoob
Copy link
Owner

@mrdoob mrdoob commented Feb 20, 2021

Related issue: #16317

Description

I think the only way of understanding the ramifications of setting "sideEffects": false is by giving it a go.

@mrdoob mrdoob added this to the r126 milestone Feb 20, 2021
@mrdoob mrdoob merged commit 87b2a7a into dev Feb 20, 2021
@mrdoob mrdoob deleted the sideeffects branch February 20, 2021 12:05
@mrdoob mrdoob mentioned this pull request Feb 20, 2021
43 tasks
@mrdoob
Copy link
Owner Author

mrdoob commented Feb 20, 2021

Interestingly, after setting sideEffects: false, the build now lose polyfills.js.
I think it's time to remove them anyway.

@mrdoob mrdoob mentioned this pull request Feb 20, 2021
@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 20, 2021

If you want to stop support for IE, I'd like to revive #18091 😇 .

@mrdoob
Copy link
Owner Author

mrdoob commented Feb 20, 2021

@Mugen87 what I was going to do next is adding the required polyfills in https://threejs.org/examples/misc_legacy.html

@mrdoob
Copy link
Owner Author

mrdoob commented Feb 20, 2021

@Mugen87 Yes, I think reviving #18091 makes sense. The clean up to CSS3DRenderer definitely. But for the common use case (WebGL) I think it'd be good if we can point people to misc_legacy.html just so they know it's possible to make it work but it's up to them to add the required polyfills.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 20, 2021

Okay, sounds fine. I'll make a new PR for CSS3DRenderer only.

Also updated the migration guide to highlight the polyfills change: https://github.com/mrdoob/three.js/wiki/Migration-Guide#r125--r126

@DefinitelyMaybe
Copy link
Contributor

DefinitelyMaybe commented Feb 20, 2021

@marcofugaro I believe with sideEffects:false we can remove the /*#__PURE__*/ comments? or was that for something else again?

@marcofugaro
Copy link
Contributor

that's a question for @ianpurvis, could you test with shakediff if the /*@__PURE__*/ annotations are still needed? 🙏

@mrdoob
Copy link
Owner Author

mrdoob commented Feb 23, 2021

@Mugen87 I think after d401181 and b4b24e3, #18091 is done?

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 23, 2021

Yes, indeed! There are some minor documentation changes left but these can be handled later.

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

Successfully merging this pull request may close these issues.

4 participants