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

[@mapbox/static-apis] [feature] Provide modules with reference without docs and examples #11628

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ahocevar
Copy link
Contributor

@ahocevar ahocevar commented Mar 19, 2022

This pull request adds the flow-stripped source code of the @mapbox/mapbox-gl-style-spec package to the dist/ folder and adds them as exports to package.json. The latest reference imports a minified version of the v8 reference, without docs and examples.

The goal of the separate modules is to allow third-party applications to create smaller builds, and the motivation was that the ol-mapbox-style library currently has a full build with a size of 638 kB. When using the modules and minified reference from this pull request, the build size can be reduced to 534 kB.

The @mapbox/mapbox-gl-style-spec package now exports separate modules in addition to the ESM and CJS bundles. The difference between the bundles and the modules is that the latter use a minified version of the latest style refererence, which does not contain any doc and example fields. See the exports section of package.json for available modules.

Launch Checklist

  • briefly describe the changes in this PR (see above)
  • include before/after visuals or gifs if this PR includes visual changes (n/a)
  • write tests for all new functionality (n/a)
  • document any changes to public APIs (no changes)
  • post benchmark scores (n/a)
  • manually test the debug page
  • tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port (none)
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog></changelog>

@CLAassistant
Copy link

CLAassistant commented Mar 19, 2022

CLA assistant check
All committers have signed the CLA.

@ahocevar ahocevar force-pushed the style-spec-modules branch 2 times, most recently from ed80542 to f47b20d Compare March 23, 2022 09:03
@ahocevar ahocevar marked this pull request as draft March 23, 2022 10:33
@ahocevar ahocevar force-pushed the style-spec-modules branch 6 times, most recently from 7fdd2d5 to 4173dbb Compare March 23, 2022 12:25
@ahocevar ahocevar marked this pull request as ready for review March 23, 2022 15:18
@ahocevar
Copy link
Contributor Author

ahocevar commented Mar 23, 2022

This is now ready for review.

@mourner
Copy link
Member

mourner commented Mar 31, 2022

The difference between the bundles and the modules is that the latter use a minified version of the latest style refererence, which does not contain any doc and example fields.

Sorry for a delay with response. If this is the root of the issue, perhaps a more reliable solution would be to fix it directly — e.g. by exporting both minified and full v8 references in the ESM bundle? Or is that difficult due to the way minification happens in Rollup?

@ahocevar
Copy link
Contributor Author

Thanks for the feedback, @mourner. The goal of the removal of the doc and example fields was to reduce the build size, and the reason why I did not do it in the ESM bundle is because I assume that consumers of @mapbox/mapbox-gl-style-spec may rely on the information in these fields.

It would be easy to export both minified and full v8 references in the ESM bundle, but would it meet the goal to reduce the build size of applications or libraries that depend on @mapbox/mapbox-gl-style-spec? If I'm not mistaken, bundlers only tree-shake on the module level. If that's correct, then your suggested solution would increase the build size instead of reducing it, because the full v8 reference would be carried around without actually being used.

Another solution I could think of is to include both minified and full spec as modules, but continue letting modules import the minified one. The full spec could be added e.g. as v8-full.json. Would that be an acceptable improvement to this pull request?

What exactly are your concerns regarding reliability of my proposed solution?

@mourner
Copy link
Member

mourner commented Apr 1, 2022

If I'm not mistaken, bundlers only tree-shake on the module level. If that's correct, then your suggested solution would increase the build size instead of reducing it, because the full v8 reference would be carried around without actually being used.

I don't think that's the case — the whole point of tree shaking is to only include what's used and discard the rest, module boundary or not. The only case when it's limited to module boundary is when the bundler pulls in CJS, or when ESM exports an object rather than a set of named exports, but it shouldn't be the case here.

So to sum up, let's explore adding a minified spec in the ESM exports.

@ahocevar
Copy link
Contributor Author

ahocevar commented Apr 1, 2022

Ok, let's try that. Looking at the code, I would assume that all code which depends on the reference works without docs and examples. Only the validate utility benefits from the full spec, because it will produce more useful output then.

@ahocevar
Copy link
Contributor Author

ahocevar commented Apr 1, 2022

@mourner I created #11661 implementing what you suggested. It was a bit more complicated because i did not want to modify the build process of mapbox-gl-js, so I added a replacer in rollup.config.js that actually replaces the build tool/configuration agnostic min export with the one that the rollup config creates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants