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

chore: Update to Node 20 #6385

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

chore: Update to Node 20 #6385

wants to merge 24 commits into from

Conversation

snowystinger
Copy link
Member

@snowystinger snowystinger commented May 15, 2024

Closes #6380

Not sure why, but the SSR tests are super brittle now. In addition, I haven't gotten the make website-production working locally, would love someone else to run those as well.
Just make sure every terminal you open is set to node 20.15.0

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

@rspbot
Copy link

rspbot commented Jun 24, 2024

@rspbot
Copy link

rspbot commented Jun 24, 2024

1 similar comment
@rspbot
Copy link

rspbot commented Jun 24, 2024

@rspbot
Copy link

rspbot commented Jun 24, 2024

@snowystinger snowystinger changed the title Update to Node 22 Update to Node 20 Jun 26, 2024
@devongovett devongovett force-pushed the main branch 2 times, most recently from 8cb1a5d to 3013156 Compare July 23, 2024 22:43
@rspbot
Copy link

rspbot commented Sep 27, 2024

@rspbot
Copy link

rspbot commented Sep 27, 2024

@rspbot
Copy link

rspbot commented Sep 27, 2024

1 similar comment
@rspbot
Copy link

rspbot commented Sep 27, 2024

@snowystinger snowystinger marked this pull request as ready for review September 27, 2024 07:24
@rspbot
Copy link

rspbot commented Sep 27, 2024

@rspbot
Copy link

rspbot commented Sep 27, 2024

@devongovett devongovett force-pushed the main branch 2 times, most recently from a79adcf to 3013156 Compare September 30, 2024 21:00
@rspbot
Copy link

rspbot commented Oct 14, 2024

Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

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

Running make website-production right now, but otherwise I'm not sure I have enough background about the changes made in this PR.

EDIT: Got the following error when running make website-production, I assume that was expected from the comment in your description?

@parcel/transformer-postcss: Could not resolve module 
"@spectrum-css/component-builder/css/processors" from 
"/private/var/folders/s9/z051ddk558z1778tc9fx1n380000gq/T/177329403da46b96590bfd204cd422a6/postcss.config.js"

@@ -34,7 +34,7 @@ module.exports = {
'packages/'
],

testTimeout: 30000,
testTimeout: 50000,
Copy link
Member

Choose a reason for hiding this comment

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

oof, was it the SSR tests you mentioned that prompted this?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I'm not sure why they take longer now

Copy link
Member Author

Choose a reason for hiding this comment

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

attempting to fix with maxConcurrency instead, might just be something with how node does concurrency now

Copy link
Member

Choose a reason for hiding this comment

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

whats the story with this?

Copy link
Member Author

Choose a reason for hiding this comment

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

spectrum-css/builder had a whole bunch of dependencies we weren't making use of, even transitively, so I got rid of them by making the temp version
it also allowed me to get rid of a patch

@snowystinger
Copy link
Member Author

EDIT: Got the following error when running make website-production, I assume that was expected from the comment in your description?

yep, that's the same one I was getting. I'm not sure why the resolver isn't working...

# Conflicts:
#	package.json
#	yarn.lock
@@ -108,6 +108,19 @@ class NumberParserImpl {

constructor(locale: string, options: Intl.NumberFormatOptions = {}) {
this.locale = locale;
// see https://tc39.es/ecma402/#sec-setnfdigitoptions, when using roundingIncrement, the maximumFractionDigits and minimumFractionDigits must be equal
Copy link
Member Author

Choose a reason for hiding this comment

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

note, this previously threw in Chrome and Node 22
new Intl.NumberFormat('en-US', {roundingIncrement: 2})

@snowystinger snowystinger changed the title Update to Node 20 chore: Update to Node 20 Dec 1, 2024
# Conflicts:
#	yarn.lock
@rspbot
Copy link

rspbot commented Feb 7, 2025

@rspbot
Copy link

rspbot commented Feb 7, 2025

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.

Heads-up: repo can't be installed with Node.js 22
3 participants