Skip to content

Conversation

@Joezo
Copy link
Contributor

@Joezo Joezo commented Feb 27, 2019

Co-authored-by: Jack Clark

What does this PR do?

Generates bundles for UMD, ESM and CommonJS. This brings with it support for browsers >= IE11.

Whilst working on this we published a few version to npm with the test tag so that it could be tested. This codepen has been created which uses the UMD build. We spun up a quick Create react app to test out ESM which worked great. The nextjs example app has also been updated locally to use the commonjs version.

Our rollup config was heavily influenced by the one in redux.

Bundle size has slightly increased but we now have increased browser support and our module is now tree shakeable, meaning potentially reduced size for end users.

Important
We now use Promises to write this library as transpiling from async/await is usually avoided in libraries as it's expensive. We opted instead to use untranspiled Promises and then inform the user they should provide an environment that includes Promises.

Related issues

Resolves #11

Checklist

  • I have checked the contributing document
  • I have added or updated any relevant documentation
  • I have added or updated any relevant tests

bmullan91
bmullan91 previously approved these changes Feb 27, 2019
Copy link
Contributor

@bmullan91 bmullan91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Joezo / @jackdclark LGTM - One question about async/await.

@Joezo Joezo force-pushed the rollup branch 2 times, most recently from 62cb4d3 to c7f7cfa Compare February 27, 2019 15:58
@Joezo Joezo requested a review from bmullan91 February 27, 2019 15:59
Copy link
Contributor

@bmullan91 bmullan91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only a few small things - everything else is perfect 👌

"contributors:generate": "all-contributors generate"
"contributors:generate": "all-contributors generate",
"build": "rollup -c",
"prepublishOnly": "npm run build"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to run npm run build in prepack also.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add it in prepack I believe it will run twice when we run npm publish. Are we using npm pack?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think npm pack might be an edge case and if people want to use it they could do npm run build first?

Copy link
Contributor

@bmullan91 bmullan91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :shipit:

@Joezo Joezo merged commit ae154d3 into master Feb 28, 2019
@Joezo Joezo deleted the rollup branch February 28, 2019 10:45
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.

4 participants