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

[WIP] expose ESM browser build and remove fetch and Promise polyfills - fixes #57 and #59 #60

Merged
merged 7 commits into from
May 15, 2020

Conversation

nsivertsen
Copy link
Contributor

@nsivertsen nsivertsen commented Sep 4, 2018

Background

As mentioned in #59, prismic-javascript currently only exports a UMD build that is not tree-shakeable by bundlers and therefore forces consumers of the library to include everything in their bundles, instead of only the functions they actually use.

In addition, as mentioned in #57, prismic-javascript currently forces down polyfills for fetch and Promise. By now, everyone know that JS is the most expensive [1], [2] resource on most websites. Since browser support for fetch (87.1%) and Promise (88.85%) is actually quite good, and most real-world projects already have some polyfilling strategy (such as polyfill.io) in place (meaning that in the worst case, users are forced to download the polyfills twice), it feels a little unnecessary to include them here.

I've therefore removed promise-polyfill and replaced cross-fetch with node-fetch (which is used internally by cross-fetch when run on Node), which uses window.fetch when bundled for the browser. Even before tree-shaking, this already reduces the bundle size of prismic-javascript.min.js from 31.9KB to 13.5KB.

Description

Aside from the existing dist/prismic-javascript.min.js UMD build, this PR adds the following builds:

  • dist/prismic-javascript.js: CJS build for Node.js
  • dist/prismic-javascript.mjs: ESM build for Node.js
  • dist/prismic-javascript.browser.js: UMD build for browsers
  • dist/prismic-javascript.browser.mjs: ESM build for browsers

It uses the package.browser field, which is understood by all major bundlers (Webpack, Rollup, Browserify), to provide the browser bundles.

Since Webpack still doesn't support outputting ES modules, I was forced to switch to Rollup, but I think you'll find the change quite pleasant as builds are faster and the configuration file is simpler (especially when considering it now outputs five bundles rather than one).

Unfortunately, because polyfills are no longer shipped with the bundle, this is potentially a breaking change for some people (but Semver protects us from that), but one I hope you'll agree is favourable in the long-term and might consider including in the upcoming 2.0 release. 🤞

There are some things left to do (see below), but I thought I'd leave this here already for discussion.

Thank you!

Todo

  • rename dist/prismic-javascript.esm.js to dist/prismic-javascript.mjs for Node compatibility
  • adapt tests to new build outputs
  • add section on polyfills to README
  • add section on bundling for node and browsers to README

Fixes #57, fixes #59

@berstend
Copy link

berstend commented Sep 13, 2018

Hey @nsivertsen thanks a lot for this PR!

I came across it while searching for the best way to support multiple environments and it was very helpful.

I blatantly copied some parts of the rollup config for one of my projects. 😄

I would recommend adding a unpkg property to the package.json pointing to the .min.js file as well.

Thanks again!

edit, just noticed you're in Berlin - happy to buy you a coffee sometime 😄

@hypervillain
Copy link
Contributor

Hey @nsivertsen,

thank you so much for your work, I'm sorry that our team did not take care of this earlier. I'll merge this and write the documentation in the coming days!

@MarcMcIntosh MarcMcIntosh mentioned this pull request Oct 31, 2019
4 tasks
@nsivertsen
Copy link
Contributor Author

@hypervillain oh, I didn't expect this to still be merged. That's great news! I see you've also removed Ramda from prismic-richtext. This means I will finally be able to include Prismic SDKs in my bundles. (I've been proxying Prismic APIs on the server and doing all rich-text processing there because of the large bundle size). Thanks!

@MarcMcIntosh
Copy link
Contributor

hi @nsivertsen we do have some questions about a few of the todo's left in the readme.
It would be great if you had the time to go over the details.

@nsivertsen
Copy link
Contributor Author

hi @MarcMcIntosh I don't have much time before next week, unfortunately, but perhaps it's an easy question?

Are you referring to "add section on bundling for node and browsers to README"?

I saw that you're requiring fetch to be polyfilled on Node. I don't believe that's the correct approach, as it requires consumers of the library to patch globals. I think it might make sense to create a build-time static boolean (through rollup-plugin-replace) to detect whether we're bundling for the server, and require('node-fetch') in that case. rollup's tree-shaking should take care of removing it for the browser version. So something like:

let fetch = window && window.fetch;

if (SERVER_SIDE) {
    fetch = require('node-fetch');
}

export default fetch;

And then import the above wherever fetch() is used.

Note that I haven't tried the above, but I think it should work.

@MarcMcIntosh
Copy link
Contributor

Hi @nsivertsen that's exactly what I meant :)
For poly-fills is it only a Promise and fetch poly-fill that is required?

@nsivertsen
Copy link
Contributor Author

@MarcMcIntosh Promise and fetch are the only two features that were polyfilled in the previous version of the library, so it seems that is the case

@MarcMcIntosh
Copy link
Contributor

@nsivertsen That's good to know,
also I have a question about adding a section for bundling for node and browsers to README.
Are there any intricacies a user should be made aware of?

@nsivertsen
Copy link
Contributor Author

@MarcMcIntosh As mentioned above, I wouldn't require anything special in Node (and rather include node-fetch by default, since it will always be needed). For browsers, it should be stated that polyfills for fetch and Promise are required if the user is targeting older browsers.

Also, for people not using standard bundlers such as webpack or rollup, it might be helpful to know that module resolution for browsers relies on the browser field in package.json:

{
  "main": "dist/prismic-javascript.js",
  "module": "dist/prismic-javascript.mjs",
  "browser": {
    "./dist/prismic-javascript.js": "dist/prismic-javascript.browser.js",
    "./dist/prismic-javascript.mjs": "dist/prismic-javascript.browser.mjs"
  }
}

How this works is that Node code that uses CommonJs gets the main field, while Node code using ESM and bundled e.g. with rollup will get the module field.

When bundling for the browser, we want the code without node-fetch, etc., which is achieved by overriding the entry points using the browser field. This is a standard package.json field: https://docs.npmjs.com/files/package.json#browser

Webpack will automatically check the browser field, so nothing needs to be done there: https://webpack.js.org/configuration/resolve/#resolvealiasfields

Rollup, on the other hand, won't. It will need to be configured in @rollup/plugin-node-resolve using the `browser option: https://github.com/rollup/plugins/tree/master/packages/node-resolve#browser

I hope that helps!

@MarcMcIntosh
Copy link
Contributor

@nsivertsen Thanks for clearing that up, we will need to update the read me so users can have that information available to them :)

@hypervillain hypervillain merged commit b97af55 into prismicio:master May 15, 2020
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.

Output tree-shakeable module Consider removing polyfills
4 participants