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

[Build] Revisit polyfill strategy #6209

Closed
wants to merge 12 commits into from

Conversation

jtoar
Copy link
Contributor

@jtoar jtoar commented Aug 13, 2022

This PR

  1. bumps the version of Node.js Redwood's build process targets from 12.x to 14.x, and
  2. reduces the number of polyfills (specifically on the api side) by using the babel-plugin-polyfill-corejs3 usage-pure strategy explicitly (99% of the changes are due to this)

Note

This PR builds on #6208.

When I started this working on this, it was just to bump the Node.js version Redwood targets from 12.x to 14.x. This took about two minutes, but I looked at the changes in .redwood/prebuild, and while the syntax transformations took effect (Bbael left ?? alone) none of the polyfills were gone, even though things like String.prototype.matchAll were in Node.js 14. So I did some digging.

Right now, we polyfill the kitchen sink. The strategy we use, plugin-transform-runtime, doesn't take the version of Node.js we target into account. It just polyfills as much as it can, including the the trivial methods that have been in Node.js since v0.11.0 like Array.prototype.forEach. Note that I'm mainly talking about the api side. The web side actually does take the target into account because it uses a different polyfill strategy, but we target a lot of browsers.

Babel's been rethinking their polyfill story and there's a newish (it's been around for a while actually, and is used under-the-hood of the one we already use, just without the customizations we need) plugin we can leverage: babel-plugin-polyfill-corejs3. This plugin takes the Node.js target into account, and if we use the usage-pure strategy, 1) doesn't polyfill things we don't use and 2) polyfill in a pure (i.e. doesn't pollute the global scope by doing an "effectful" import) way.

babel-plugin-polyfill-corejs3's usage-pure strategy isn't perfect. For example, it's not detecting that Promise.any needs to be polyfilled. The solution to this is to use the includes option to completely polyfill a few builtins (which is what we're doing now, just for everything instead of for a curated few). We just have to keep track of them.

Also, the api side was pinned to core-js 3. It's a best practice to pin it to the minor version of core-js, not the major. But if we do that, we're going to polyfill a lot more proposals (cause we have proposals set to true). See the end of this comment #6208.

Further reading:

@jtoar jtoar added release:breaking This PR is a breaking change next-release/major labels Aug 13, 2022
@jtoar jtoar self-assigned this Aug 13, 2022
@nx-cloud
Copy link

nx-cloud bot commented Aug 13, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 6f69b74. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 14 targets

Sent with 💌 from NxCloud.

@netlify
Copy link

netlify bot commented Aug 13, 2022

Deploy Preview for redwoodjs-docs canceled.

Name Link
🔨 Latest commit 6f69b74
🔍 Latest deploy log https://app.netlify.com/sites/redwoodjs-docs/deploys/62fe050accf8e100089f1ff8

Comment on lines +99 to +101
assumptions: {
enumerableModuleMeta: true,
},
Copy link
Contributor Author

@jtoar jtoar Aug 13, 2022

Choose a reason for hiding this comment

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

There's more we could probably do here, but it hasn't been my main focus. For docs on this one see https://babeljs.io/docs/en/assumptions#enumerablemodulemeta

babel.config.js Outdated
Comment on lines 32 to 108
targets: { node: TARGETS_NODE },
useBuiltIns: 'usage',
corejs: {
version: CORE_JS_VERSION,
// List of supported proposals: https://github.com/zloirock/core-js/blob/master/docs/2019-03-19-core-js-3-babel-and-a-look-into-the-future.md#ecmascript-proposals
proposals: true,
},
// See https://babeljs.io/docs/en/babel-preset-env#shippedproposals.
shippedProposals: true,
Copy link
Contributor Author

@jtoar jtoar Aug 13, 2022

Choose a reason for hiding this comment

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

I still have to scan our packages to see if we're using a lot of proposals, but I don't think we are. Regardless, we have to make a decision if we want to use them because it opens the door to a lot of Stage 0-2 features that could change/get dropped. shippedProposals is a nice compromise because it allows using proposals that are Stage 4+. And we could always just include the proposals we want to use in an explicit includes array, so it's not all or nothing.

@jtoar jtoar force-pushed the ds-bump-babel-target,revisit-polyfills branch from 02a98eb to 86da855 Compare August 13, 2022 14:09
@jtoar jtoar marked this pull request as ready for review August 14, 2022 14:30
@jtoar jtoar force-pushed the ds-bump-babel-target,revisit-polyfills branch from e486fd3 to 03047cb Compare August 18, 2022 07:29
@jtoar jtoar force-pushed the ds-bump-babel-target,revisit-polyfills branch from 03047cb to 9058366 Compare August 18, 2022 07:29
@jtoar
Copy link
Contributor Author

jtoar commented Aug 18, 2022

I need to double check a few things, but I want to get deploy-target CI testing this one before getting sinking more time into this. I'm going to merge this and then I'll revert it later if deploy target CI blows up, but regardless I'll follow up with a PR that cleans up a few things.

packages/internal/src/build/babel/api.ts Outdated Show resolved Hide resolved
packages/internal/src/build/babel/api.ts Outdated Show resolved Hide resolved
packages/internal/src/build/babel/common.ts Outdated Show resolved Hide resolved
packages/internal/src/__tests__/build_api.test.ts Outdated Show resolved Hide resolved
@jtoar jtoar marked this pull request as draft August 18, 2022 12:41
@jtoar jtoar changed the title [Build] Bump Node.js target, revisit polyfills [Build] Revisit polyfill strategy Sep 2, 2022
@jtoar jtoar mentioned this pull request Oct 7, 2022
5 tasks
@jtoar
Copy link
Contributor Author

jtoar commented Dec 22, 2023

See #7712.

@jtoar jtoar closed this Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:breaking This PR is a breaking change
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants