search

mapbox / underreact

A light weight app build system
Other
16 stars 4 forks source link

Support mapbox-gl 2.0 #108

Open samanpwbb opened 3 years ago

samanpwbb commented 3 years ago

Out of the box, underreact does not support the latest version of mapbox-gl. Since underreact is often used by apps that use Mapbox, the two projects should be compatible. Read more about why mapbox-gl breaks here: https://github.com/mapbox/mapbox-gl-js/issues/10173

The basic problem is related to transpiling gl js's worker code:

Babel correctly transpiles the inlined worker, however it relies on injecting a bunch of code into the global-scope which is a part of its runtime. Since the gl-js workers are inlined, build tools in the application (like studio) are not able to recognize this. This leaves the worker bundle with transpiled code, but missing global scope functions.

There are two possible fixes, documented here:

  1. Ignore transpiling GL JS
  2. Transpile GL JS, but use the external worker file.

I suggest option 1: It saves us bundle size and may also speed up time to first render (it's 31% lighter, saving 68kb). GL JS 2.0 does not support IE11 at all anymore, so transpiling for IE11 isn't serving any purpose. Option 1 is also what GL JS team recommends.

It looks like we could fix this by updating the exclude regex in getConfigForNodeModules: https://github.com/mapbox/underreact/blob/dcb5edc64d407304c93612c62ad73890ad37a49e/lib/webpack-config/babel-loader-config.js#L81

We could also consider supporting both options, but defaulting to 1.

@kepta would love your input. This issue has a little bit of urgency, as many frontend apps at Mapbox are going to be switching to 2.0 soon.

kepta commented 3 years ago

It looks like we could fix this by updating the exclude regex in getConfigForNodeModules:

Do we know if doing this works for both gl-js v1 and v2?

samanpwbb commented 3 years ago

Do we know if doing this works for both gl-js v1 and v2?

I'm not sure but this should be pretty easy to test.

kepta commented 3 years ago

@samanpwbb I think you approach of adding gl-js v2 to exclude: /@babel(?:\/|\\{1,2})runtime/, should work. My only suggestion would be to make sure that it is only added if the consumer actually has gl-js v2, so that we dont end up breaking anything for folks using underreact v1. I guess we can do this check by checking the version of gl-js and only then adding the exclusion.

jingsam commented 3 years ago

Why not just add an new option as excludeNodeModules than a hack towards mapbox-gl? As mapbox-gl-js V2.0.1 support external worker, how underrect handle this if we want transpile mapbox-gl-js V2?

If we add special processing towards mapbox-gl, does it mean we should add support for mapbox-gl-style-spec、p-finally and more. I think underreact is a generic webpack based build tool that should not give mapbox-gl a special hack.

samanpwbb commented 3 years ago

Why not just add an new option as excludeNodeModules than a hack towards mapbox-gl? As mapbox-gl-js V2.0.1 support external worker, how underrect handle this if we want transpile mapbox-gl-js V2?

I think I agree - am going to add a second PR that does this, and defaults to including gl js in the exclude list