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

Use external @babel/runtime helpers #341

Closed
wants to merge 1 commit into from

Conversation

realityking
Copy link
Contributor

Fixes #302

It does complicate the rollup config quite a bit but this is needed to correctly generate both the ESM and CJS versions.

@trotzig
Copy link
Collaborator

trotzig commented Jan 27, 2021

Do you know if this increases the size of the built javascript file?

@realityking
Copy link
Contributor Author

realityking commented Jan 27, 2021

Do you mean the bundled file including the Babel helpers? It will probably depend on how things are build. If other libraries or the application itself uses @babel/runtime it'll be smaller, if the build pipeline is set-up for ESM and concatenating modules (web pack 4 or 5) it should be at worst the same. If it's set-up for CommonJS and there's no deduplication it will be a bit bigger because every module is wrapped.

I'll try to get some data on this :)

@realityking
Copy link
Contributor Author

To get an idea of the size impact, I created a couple of different builds of the performance tests. This is using Webpack 4, I'd expect better results with Webpack 5 or Rollup.

  • Current setup (build of src): 144K
  • ESM build with external helpers: 144K
  • CJS build with external helper: 148K
  • ESM build without external helpers: 144K
  • CJS build without external helper: 148K

Conclusion: CJS sucks but no impact on externalising these helpers :)

@realityking
Copy link
Contributor Author

@trotzig I think a release needs to be done to address #347. It'd be great if you could take a look if you're comfortable including this in that release, if not, please go ahead without this one.

],
plugins: [
babel({
babelrc: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this means that we probably want to remove the production section of the .babelrc. I can do that separately.

],
plugins: [
babel({
babelrc: false,
babelHelpers: 'bundled',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Although don't we want to change this to runtime instead of using the getBabelOutputPlugin method? I'll try out an alternative approach now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

lencioni added a commit that referenced this pull request Apr 28, 2021
Fixes #302

This is similar to #341, except it avoids having to run babel on the
rollup output, which allows us to leave the babel configuration in
the .babelrc file.
@lencioni
Copy link
Collaborator

I did this slightly differently here #353

Thanks for opening this PR!

@lencioni lencioni closed this Apr 28, 2021
@realityking
Copy link
Contributor Author

That approach is entirely fair as well. :) The different is really one of personal preference as I don't like complicated .babelrc files 😆

Thanks for getting this merged!

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