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

Bump NodeJS supported versions to ^16.16 || ^18.13 and add engines to package.json #8201

Merged
merged 15 commits into from
Jan 30, 2023

Conversation

alex-page
Copy link
Member

@alex-page alex-page commented Jan 29, 2023

WHY are these changes introduced?

NodeJS 14 is EOL and we are removing support in v11.

We currently set engines on the root package.json. The problem with this is that consumers of the library do not get this information and they can use the libraries without any warning of what version of NodeJS it supports.

WHAT is this pull request doing?

  • Increasing the supported versions of NodeJS
  • Add the engines field for NodeJS to package.json files

I would like to get NodeJS to version 16.19 before we launch version 11. However there is an issue with babel/rollup/browserslist not finding the latest version of NodeJS:

(plugin babel) BrowserslistError: Unknown version 16.19 of Node.js

How to 🎩

  • CI completes successfully

@alex-page alex-page changed the base branch from main to bump-build-deps January 29, 2023 23:17
@alex-page alex-page added 🤖Skip Changelog Causes CI to ignore changelog update check. and removed 🤖Skip Changelog Causes CI to ignore changelog update check. labels Jan 29, 2023
@alex-page alex-page marked this pull request as ready for review January 29, 2023 23:24
@github-actions
Copy link
Contributor

github-actions bot commented Jan 29, 2023

size-limit report 📦

Path Size
polaris-react-cjs 212.06 KB (0%)
polaris-react-esm 136.31 KB (0%)
polaris-react-esnext 189.85 KB (0%)
polaris-react-css 40.63 KB (0%)

### WHY are these changes introduced?

We currently set `engines` on the root `package.json`. The problem with
this is that consumers of the library do not get this information and
they can use the libraries without any warning of what version of NodeJS
it supports.

This change adds the engines field to each package to ensure our
consumers are using supported versions of NodeJS.

### WHAT is this pull request doing?

Adding the engines field to package.json files
@@ -54,7 +54,7 @@ function generateConfig({output, targets, stylesConfig}) {
/** @type {import('rollup').RollupOptions} */
export default [
generateConfig({
targets: 'extends @shopify/browserslist-config, node 12.20',
Copy link
Member Author

Choose a reason for hiding this comment

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

This scared me but nothing seems to be bad since changing the version...

Base automatically changed from bump-build-deps to v11-major January 30, 2023 00:11
Comment on lines -119 to -124
// We could omit this if we set `engines` fields properly
// As we don't set them then eslint thinks we're using node 8
'node/no-unsupported-features/node-builtins': [
'error',
{version: '>=16.0.0'},
],
Copy link
Member Author

Choose a reason for hiding this comment

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

We set the engines field now so this is no longer needed.

Copy link
Member

@sam-b-rose sam-b-rose left a comment

Choose a reason for hiding this comment

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

Looks good! I did see that the @types/node in the polaris-for-vscode/package.json is on ^14. I think it might be good to update that as well to 16 or even 18.

@alex-page alex-page changed the title 2/x Bump NodeJS supported versions to 16.16 | 18.13 2/x Bump NodeJS supported versions to ^16.16 || ^18.13 Jan 30, 2023
@alex-page alex-page merged commit 89c8ae4 into v11-major Jan 30, 2023
@alex-page alex-page deleted the bump-nodejs branch January 30, 2023 01:27
@alex-page alex-page changed the title 2/x Bump NodeJS supported versions to ^16.16 || ^18.13 Bump NodeJS supported versions to ^16.16 || ^18.13 and add engines to package.json Jan 30, 2023
Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

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

The versions chosen seem suprising to me

  • Using "^16.16.0 || ^18.13.0" for the per-package engines shall block installation on node 19 (and 20 when it comes out in April). Currently supported and future node versions seems like a bad idea.
  • Node 14 is still supported till April. I'm ok with preemptively dropping support if you feel like this shall help improve something.
  • The first LTS version of v16 was 16.13, and the first LTS version of 18 was 18.12. Is there a reason why we're choosing to claim minimum polaris versions are 16.16 and 18.13?

@@ -20,7 +20,7 @@
"shopify"
],
"engines": {
"vscode": "^1.64.0"
"node": "^16.16.0 || ^18.13.0"
Copy link
Member

Choose a reason for hiding this comment

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

This seems incorrect

@@ -31,7 +31,7 @@
},
"engine-strict": true,
"engines": {
"node": ">=14.13.1"
"node": "^16.16.0 || ^18.13.0"
Copy link
Member

Choose a reason for hiding this comment

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

By setting the engines on the specific node packages, they shall refuse to install if the node version does not match these versions. This is great for blocking installation on older unsupported versions, however the configuration of "^16.16.0 || ^18.13.0" means "Only node 16 or 18" - you're currently also blocking the installation of these packages on the currently supported node 19 (and the lts node 20 when it comes out in April).

For these per-package engine fields I strongly think we should allow current and future versions for the sake of forwards compatibility - ^16.16.0 || >=18.13.0.

sam-b-rose added a commit that referenced this pull request May 26, 2023
## @shopify/polaris v11.0.0

### Dependencies

- [x] #8200

### NodeJS

- [x] #8201

### TypeScript

- [x] #8203

### Components

- [x] #7349
- [x] #7397
- [x] #7962
- [x] #8187
- [x] #8184
- [x] #8206
- [x] #7990
- [x] #8468
- [x] #8577
- [x] #8631
- [x] #8962

## @shopify/polaris-tokens v7.0.0

### Tokens
- [x] #6920
- [x] #8245
- [x] #4826
- [x] #8405

## @shopify/stylelint-polaris v7.0.0
- [x] #7622
- [x] #8419

# Post @shopify/polaris v11 shipping
- [ ] #8420

## Low priority or not ready breaking changes
- [x] Remove deprecated layout components
- [x] Release Layout primitive components

---------

Co-authored-by: Tim Layton <[email protected]>
Co-authored-by: Ryan Musgrave <[email protected]>
Co-authored-by: Ryan Musgrave <[email protected]>
Co-authored-by: aveline <[email protected]>
Co-authored-by: Kyle Durand <[email protected]>
Co-authored-by: Matt Gregg <[email protected]>
Co-authored-by: Alex Page <[email protected]>
Co-authored-by: Lo Kim <[email protected]>
Co-authored-by: Ben Scott <[email protected]>
Co-authored-by: Aaron Casanova <[email protected]>
Co-authored-by: Sam Rose <[email protected]>
Co-authored-by: Sam Rose <[email protected]>
Co-authored-by: Marc Thomas <[email protected]>
Co-authored-by: Alex Page <[email protected]>
Co-authored-by: Chloe Rice <[email protected]>
Co-authored-by: Chloe Rice <[email protected]>
Co-authored-by: Joe Thomas <[email protected]>
Co-authored-by: Yuraima Estevez <[email protected]>
Co-authored-by: shopify-github-actions-access[bot] <109624739+shopify-github-actions-access[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
juzser pushed a commit to juzser/polaris that referenced this pull request Jul 27, 2023
## @shopify/polaris v11.0.0

### Dependencies

- [x] Shopify#8200

### NodeJS

- [x] Shopify#8201

### TypeScript

- [x] Shopify#8203

### Components

- [x] Shopify#7349
- [x] Shopify#7397
- [x] Shopify#7962
- [x] Shopify#8187
- [x] Shopify#8184
- [x] Shopify#8206
- [x] Shopify#7990
- [x] Shopify#8468
- [x] Shopify#8577
- [x] Shopify#8631
- [x] Shopify#8962

## @shopify/polaris-tokens v7.0.0

### Tokens
- [x] Shopify#6920
- [x] Shopify#8245
- [x] Shopify#4826
- [x] Shopify#8405

## @shopify/stylelint-polaris v7.0.0
- [x] Shopify#7622
- [x] Shopify#8419

# Post @shopify/polaris v11 shipping
- [ ] Shopify#8420

## Low priority or not ready breaking changes
- [x] Remove deprecated layout components
- [x] Release Layout primitive components

---------

Co-authored-by: Tim Layton <[email protected]>
Co-authored-by: Ryan Musgrave <[email protected]>
Co-authored-by: Ryan Musgrave <[email protected]>
Co-authored-by: aveline <[email protected]>
Co-authored-by: Kyle Durand <[email protected]>
Co-authored-by: Matt Gregg <[email protected]>
Co-authored-by: Alex Page <[email protected]>
Co-authored-by: Lo Kim <[email protected]>
Co-authored-by: Ben Scott <[email protected]>
Co-authored-by: Aaron Casanova <[email protected]>
Co-authored-by: Sam Rose <[email protected]>
Co-authored-by: Sam Rose <[email protected]>
Co-authored-by: Marc Thomas <[email protected]>
Co-authored-by: Alex Page <[email protected]>
Co-authored-by: Chloe Rice <[email protected]>
Co-authored-by: Chloe Rice <[email protected]>
Co-authored-by: Joe Thomas <[email protected]>
Co-authored-by: Yuraima Estevez <[email protected]>
Co-authored-by: shopify-github-actions-access[bot] <109624739+shopify-github-actions-access[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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.

3 participants