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

Shopifolk can use node v20.11.1 for development #11739

Merged
merged 1 commit into from
Mar 17, 2024
Merged

Conversation

jesstelford
Copy link
Contributor

@jesstelford jesstelford commented Mar 15, 2024

Use the latest version of node for local development. Closes https://github.com/Shopify/polaris-internal/issues/1525

NOTE: This does not increase the minimum required node version for consumers of our library. See https://github.com/Shopify/polaris-internal/issues/1524 for that.

I confirmed this does not impact the output of our build processes at all by running a build with node v18.12.0, then again with node v20.11.1, then diffing the generated files (there was no difference as expected).

Benefits

  • Perf: Every version of node gets faster improving DX & build times
  • Features: Loads of great new features we can take advantage of
  • Allows updating dev deps: Eg, Storybook v8 requires >= Node 18, etc

Performance

tl;dr: Basically no change.

Run using hyperfine:

hyperfine --parameter-list branch node-20-for-devs,main --setup='git checkout {branch} && rm -rf node_modules */node_modules\(N\) && \. ~/.nvm/nvm.sh && nvm use && yarn --prefer-offline' --prepare='rm -rf polaris-for-vscode/dist polaris-icons/dist polaris-migrator/dist polaris-react/build polaris-react/build-internal polaris-tokens/dist polaris.shopify.com/.next' --warmup 1 'yarn build'
Benchmark 1: yarn build (branch = node-20-for-devs)
  Time (mean ± σ):     12.264 s ±  2.257 s    [User: 0.838 s, System: 3.005 s]
  Range (min … max):    7.118 s … 16.111 s    10 runs

Benchmark 2: yarn build (branch = main)
  Time (mean ± σ):     12.210 s ±  1.932 s    [User: 0.831 s, System: 2.957 s]
  Range (min … max):    8.104 s … 13.980 s    10 runs

Summary
  yarn build (branch = main) ran
    1.00 ± 0.24 times faster than yarn build (branch = node-20-for-devs)

@@ -0,0 +1,54 @@
name: CI
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decoupling the running of our lint tools (which can be run in any node version) from the running of our tests (which should only be run against the versions of node we support in package.json#engines). See https://github.com/Shopify/polaris-internal/issues/1522 for the differences.

@@ -26,7 +26,6 @@
"dev": "rollup -c -w",
"lint": "run-p lint:*",
"lint:js": "TIMING=1 eslint --cache .",
"lint:styles": "stylelint '**/*.{css,scss}'",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was failing with the error:

Error: No files matching the pattern "**/*.{css,scss}" were found.

Which makes sense because there's no .css or .scss files in this folder.

But why was it working before?

Because the previous CI task would (unnecessarily) run yarn build which generates dist/**/*.css files. So, when the glob was run, there are files for stylelint to match against, and hence no error. But those files are all in dist/ which is ignored in stylelintrc.js, so stylelint would silently ignore all the files and exit successfully having done nothing.

@jesstelford jesstelford added the 🤖Skip Changelog Causes CI to ignore changelog update check. label Mar 15, 2024
@jesstelford jesstelford merged commit 8d964ae into main Mar 17, 2024
9 checks passed
@jesstelford jesstelford deleted the node-20-for-devs branch March 17, 2024 23:14
jesstelford added a commit that referenced this pull request Apr 9, 2024
Refs https://github.com/Shopify/polaris-internal/issues/1524

[Node 16 is EOL](https://nodejs.org/en/about/previous-releases), and
[Node 18 is in Maintenance
Mode](https://nodejs.org/en/about/previous-releases), so we're updating
the minimum required Node version to v20.10.0 (the latest supported by
`node-releases`/`browserslist` at the time this project started).

## When is node used?

Node is used in 2 contexts:

1. By our consumers when they import, run, and bundle our libraries
2. By us, the Polaris team, while we develop our libraries

These are very different, and have their own configs within our
repository...

### 1. Consumer's node version

When a consumer uses one of our libraries, they're importing whatever is
in the `build/*` folder. Those assets are generated by our build process
(Rollup) which transforms (via babel) whatever version of Javascript we
write our code in into a version that's compatible with the [`targets`
setting we
provide](https://github.com/Shopify/polaris/blob/9c24a465c5e001e148bde335bf6319e924f4b1d6/polaris-react/rollup.config.mjs#L72).

For example, `Array#findLast` was only made available in node v18, so
when we write code like:

```javascript
[1, 2, 3, 2, 1].findLast(x => x < 2);
```

The output will be different depending on our Babel config:

```javascript
// with babel config: { targets: 'node 16.0' }
import "core-js/modules/esnext.array.find-last.js";
[1, 2, 3, 2, 1].findLast(x => x < 2);
```

```javascript
// with babel config: { targets: 'node 18.0' }
[1, 2, 3, 2, 1].findLast(x => x < 2);
```

_(try it
[here](https://babeljs.io/repl#?browsers=&build=&builtIns=usage&corejs=3.21&spec=false&loose=false&code_lz=NoRgNABATJDMkwiAugOgGYEsB2ATAMgIYDOALgBQAeEAvAHwTUA80AlANxA&debug=false&forceAllTransforms=false&modules=false&shippedProposals=false&circleciRepo=&evaluate=false&fileSize=false&timeTravel=false&sourceType=module&lineWrap=true&presets=env&prettier=false&targets=Node-16.0&version=7.24.0&externalPlugins=&assumptions=%7B%7D)
- set the "Env preset > Node" version to `18.0`)_

The `node 18.0` example above does not include a polyfill for
`Array#findLast`, which means if our consumers try to run Polaris in a
node environment (eg; during SSR) that doesn't support `Array#findLast`,
they will get an error.

The way we let our consumers know what is the minimum version of node
they can use is via the `packages.json#engines.node` field. As long as
this matches the version specified in `Babel#targets`, any consumer
trying to install our library in an unsupported version of Node will
receive an error.

This PR updates the minimum required node version of our consumers to
`20.10.0`.

### 2. Our node version while developing Polaris

Similarly, the tools we use to develop Polars (Babel, Postcss, etc) may
have a minimum node version requirement.

They do it by specifying a `package.json#engines.node` version in their
own library. Then when we install their package, if our version of node
isn't compatible we'll receive an error.

However, the version of node required to run Babel _has no impact on its
ability to transform our code to work on older versions of node_. As
long as we tell Babel which version of node to output code for in
`targets`, it doesn't matter what our `.nvmrc` file says.

#11739 updated the node version
for developing with Polaris to `20.11.1`.

---------

Co-authored-by: Aaron Casanova <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖Skip Changelog Causes CI to ignore changelog update check.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants