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

Remove side effects from core module #3395

Merged
merged 3 commits into from Aug 5, 2019
Merged

Conversation

Pessimistress
Copy link
Collaborator

@Pessimistress Pessimistress commented Aug 2, 2019

Background

Per discussion in #3213, sideEffects is not sufficient to prevent tree shaking in some use cases.

Change List

  • Wrap global registration in a function

Tested with build-bundle script.

@Pessimistress Pessimistress changed the title X/no side effects Remove side effects from core module Aug 2, 2019
@coveralls
Copy link

coveralls commented Aug 2, 2019

Coverage Status

Coverage increased (+0.009%) to 83.014% when pulling e77402c on x/no-side-effects into ca074e2 on master.

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The source code structure becomes a little weird this way.

Unless we think we can get the global inits working again later, I wonder if we should just move those init functions into deck.js (or at least lib folder).

I could see some issues in that one might for instance want to call loaders.gl load on something before creating deck and now one does get different loaders before and after deck is instantiated.

But this is still the lesser evil, as we have get this darned tree-shaking issue fixed...

@Pessimistress
Copy link
Collaborator Author

I could see some issues in that one might for instance want to call loaders.gl load on something before creating deck and now one does get different loaders before and after deck is instantiated.

That's fair; I have pushed an alternative approach. It should make no change to the current behavior.

@Pessimistress Pessimistress merged commit 134e801 into master Aug 5, 2019
@Pessimistress Pessimistress deleted the x/no-side-effects branch August 5, 2019 22:17
@SterlingMcCall
Copy link

Due to the fact that this was/is a rather inconvenient bug for those affected, would it be possible to release this fix as version 7.1.11 so we don't have to wait until the release of 7.2.0?

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.

None yet

4 participants