-
Notifications
You must be signed in to change notification settings - Fork 120
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
Use transpiled imports from mapbox-gl-style-spec #94
Conversation
Agreed @ahocevar, since CRA is adding support for transpiling node_modules in 2.0, we don't need to merge this. |
I've tried this with CRA 2.0 this morning and I don't think it solves the problem directly. Or at least the way Webpack is configured is not doing transpiling the deeper modules. I'm getting the same syntax errors from that I was getting with CRA 1.x. |
Can you point me to an example project so I can reproduce the issue and investigate? Thanks! |
hey @ahocevar basically the instructions here: https://www.npmjs.com/package/@boundlessgeo/sdk just make sure to leave out the version numbers for ol-mapbox-style and sdk |
I just came across this same issue trying to use |
Installing |
That version uses older versions of the Mapbox dependencies, without support for style expressions. So, how should we proceed? Do we accept bundle sizes that are 10kB larger than they need to be, and merge this pull request? I tend to say yes now that create-react-app still does not work with this library out of the box, and neither does Parcel. Other opinions? |
I also tend to say yes |
see also: mapbox/mapbox-gl-js#7105 |
@ahocevar any objections to merging this one? |
At this point it's 10kb vs a lot of broken apps... |
Let‘s do this. |
This pull request replaces #92.
The downside is that all bundles created from
ol-mapbox-style
will be larger by 10 kB than with the source imports we previously used.For
create-react-app
users, we should consider making that work with ES modules, which it maybe already does in the latest alpha version. See facebook/create-react-app#3889.That said, I'm not sure if we want to merge this. @bartvde @frankrowe what do you think?