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

Introduce static worker version of the bundle for strict CSP environments #8044

Merged
merged 6 commits into from
Mar 21, 2019

Conversation

mourner
Copy link
Member

@mourner mourner commented Mar 15, 2019

Closes #6058. Adds a build-csp script that builds a static worker version of the GL JS bundle — dist/mapbox-gl-csp.js and dist/mapbox-gl-csp-worker.js, which would be included in the distribution of the next release. The only additional step to using this bundle is setting mapboxgl.workerUrl manually before instantiating the map:

<script src='/dist/mapbox-gl-csp.js'></script>
<script>
    mapboxgl.workerUrl = '/dist/mapbox-gl-csp-worker.js';
    var map = new mapboxgl.Map(...);

You can test this out locally with the debug/csp-static.html page.

Bundle size comparison

bundle minified gzipped
mapbox-gl.js 656.74KB 166.76KB
mapbox-gl-csp.js 597.66KB 147.30KB
mapbox-gl-csp-worker.js 324.46KB 85.42KB

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page

@mourner
Copy link
Member Author

mourner commented Mar 15, 2019

Feel free to suggest different naming — maybe mapbox-gl.static.js would be better?

@ansis
Copy link
Contributor

ansis commented Mar 20, 2019

This looks good to me.

Is the script loaded only when the first worker is initialized? Is this ok or would first load be faster if we prefetch it? This is something we don't need to address now.

The only additional step to using this bundle is setting mapboxgl.workerUrl manually before instantiating the map

Is it possible to default this to the path the current script was loaded from? So that if a user puts both files in the same directory and doesn't rename them they don't need to specify this?

Feel free to suggest different naming — maybe mapbox-gl.static.js would be better?

I think both files should share a name to show that they go together. For example,
mapbox-gl.ASDF.js and mapbox-gl.ASDF.worker.js (or mapbox-gl.ASDF-worker.js).

Other possible names are csp-worker-src to be more specific about what this solves. Or no-worker-blob. Or just split?

mapbox-gl.split.js and mapbox-gl.split.worker.js?

@mourner
Copy link
Member Author

mourner commented Mar 20, 2019

Is the script loaded only when the first worker is initialized? Is this ok or would first load be faster if we prefetch it? This is something we don't need to address now.

If the map isn't instantiated on load, this can be addressed on the app side with:

<link rel="preload" href=".../mapbox-gl-worker.js" as="script">

Is it possible to default this to the path the current script was loaded from? So that if a user puts both files in the same directory and doesn't rename them they don't need to specify this?

We could, but I think that with the current prevalence of bundlers and fragility of regexp script heuristics, it's not worth doing. If someone wants to switch to this special bundle, I think they won't have a problem adding a short line of code, and it will be reliable.

I think both files should share a name to show that they go together.

Good point! I can rename to mapbox-gl-csp.js and mapbox-gl-csp-worker.js (using hyphens for consistency with other existing builds). I think we should keep the names simple (mapbox-gl-no-worker-blob-worker.js looks scary), but in that case at least csp gives a clue about what the bundle is about (split without context seems more cryptic).

@ansis
Copy link
Contributor

ansis commented Mar 20, 2019

👍

@mourner mourner merged commit 4cdbf11 into master Mar 21, 2019
@mourner mourner deleted the static-worker branch March 21, 2019 08:51
@thomasneirynck
Copy link

Thanks @mourner!

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.

3 participants