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

Ignore mapbox-gl transpiling #217

Merged
merged 1 commit into from
Mar 21, 2022

Conversation

torosyaneric
Copy link
Collaborator

Resolves #213

@torosyaneric
Copy link
Collaborator Author

@justinanderson I think not transpiling Mapbox is still better solution than transpiling Web Worker separately, as they say it significantly increases bundle size and negatively impacts rendering - is only considered if targeting legacy browsers.

If your application requires ES5 compatibility, then your module bundler needs to be configured to load and transpile Mapbox GL JS's Web Worker separately. This comes at the cost of significantly increasing the bundle size and negatively impacting rendering performance and you should only do this if you have a strong need for supporting legacy browsers. Mapbox GL JS can be configured with bundler specific worker-loader plugins.

@justinanderson
Copy link
Member

@torosyaneric But do we require ES5 compatibility? How does this compare with Mapbox's first suggestion to use browserlist?

@torosyaneric
Copy link
Collaborator Author

@justinanderson it seems the browserslist approach wasn't working as our browserslist value is identical to their suggestion (+ "not op_mini all")

@justinanderson
Copy link
Member

@torosyaneric Did you see this comment mapbox/mapbox-gl-js#10565 (comment) and the ones referencing it later from the link in the issue? It looks like Mapbox's documentation may need an update. Our production builds work with that modification, >0.2% -> >0.2% and supports es6-class.

@torosyaneric
Copy link
Collaborator Author

torosyaneric commented Mar 17, 2022

@justinanderson sorry, missed that one, fixed and pushed. Thanks for mentioning this.

@justinanderson
Copy link
Member

@torosyaneric But, I mean, what's the difference between the two options? I don't have much experience with transpilation concerns and would like to understand the choice we're making.

@torosyaneric
Copy link
Collaborator Author

@justinanderson I'm not 100% percent sure but I suppose using browserslist approach prevents the transpilation for whole codebase by rule "supports es6-class" and "import from !mapbox" was just ignoring the mapbox transpilation. I don't think these have advantages in terms of bundle size, maybe in build time.
Do you want me to investigate this further?

@justinanderson
Copy link
Member

@torosyaneric Nah, this is fine. I spent some time to learn about transpilation and feel I understand it well enough now.

It looks like we can safely ignore browsers which don't support ES6, so I'll take this as is.

@justinanderson justinanderson force-pushed the SA-213-ignore-mapbox-transpiling branch from 06ec2df to 8e6f11c Compare March 21, 2022 15:38
@justinanderson
Copy link
Member

Force-pushed to remove noop commit and allow this branch to rebase cleanly.

@justinanderson justinanderson merged commit 4d96b8d into master Mar 21, 2022
@justinanderson justinanderson deleted the SA-213-ignore-mapbox-transpiling branch March 21, 2022 15:40
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.

Address Mapbox transpilation errors in production builds
2 participants