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.

Sign up for GitHub

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

Jump to bottom

Update Node.js and vscode #19194

Merged
merged 1 commit into from
Feb 27, 2025
Merged

Update Node.js and vscode #19194

merged 1 commit into from
Feb 27, 2025

Conversation

BenjaminBrienen
Copy link
Contributor

@BenjaminBrienen BenjaminBrienen commented Feb 21, 2025 •
edited
Loading

  • Updated Node to 22 (targeting node20 for the sake of matching vscode 1.97)
  • Use flat eslint config
  • Use recommended eslint configs
  • Make lints and prettier play nice with each other
  • Update code to follow lints
    • Ignored some any stuff because I don't know if it can be fixed
  • Updated deps
  • Added jiti because it is required for TS config

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 21, 2025
lnicola
lnicola reviewed Feb 21, 2025
View reviewed changes
@lnicola
Copy link
Member

lnicola commented Feb 21, 2025

https://eslint.org/docs/latest/use/configure/migration-guide might help here, but you've probably found it already.

@BenjaminBrienen
Copy link
Contributor Author

Yeah, just was struggling with it at 3 am. I'll fix it 😅

@BenjaminBrienen BenjaminBrienen changed the title Update Node.js and package.json deps Update Node.js and everything that entails Feb 23, 2025
@BenjaminBrienen
Copy link
Contributor Author

Some proposals must have been merged

Via 'product.json#extensionEnabledApiProposals' extension 'ms-vscode.vscode-selfhost-test-provider' wants API proposal 'attributableCoverage' but that proposal DOES NOT EXIST.

@BenjaminBrienen BenjaminBrienen changed the title Update Node.js and everything that entails Update Node.js and vscode Feb 23, 2025
@BenjaminBrienen BenjaminBrienen changed the title Update Node.js and vscode Update Node.js Feb 23, 2025
@BenjaminBrienen
Copy link
Contributor Author

I removed the vscode update and now I'm getting a different error in the same test:

Via 'product.json#extensionEnabledApiProposals' extension 'ms-dotnettools.dotnet-interactive-vscode' wants API proposal 'languageConfigurationAutoClosingPairs' but that proposal DOES NOT EXIST. Likely, the proposal has been finalized (check 'vscode.d.ts') or was abandoned.
Via 'product.json#extensionEnabledApiProposals' extension 'github.copilot

it seems like it is trying to use extensions such as copilot chat in the test?

@BenjaminBrienen BenjaminBrienen changed the title Update Node.js Update Node.js and vscode Feb 23, 2025
@BenjaminBrienen
Copy link
Contributor Author

ok, new vscode is actually working now

@BenjaminBrienen
Copy link
Contributor Author

Error: Only URLs with a scheme in: file, data, node, and electron are supported by the default ESM loader. On Windows, absolute paths must be valid file:// URLs. Received protocol 'd:'

I can reproduce this error on my windows machine. I don't really know where this is coming from. There is a missing file:// somewhere in windows-specific code, I guess.

@BenjaminBrienen
Copy link
Contributor Author

wow, I can't believe I actually figured out where that bug was happening. there we go!

@BenjaminBrienen
Copy link
Contributor Author

I hate javascript

@BenjaminBrienen
Copy link
Contributor Author

@lnicola I think this is ready for your review :)

davidbarsky
davidbarsky previously requested changes Feb 23, 2025
View reviewed changes
lnicola
lnicola approved these changes Feb 23, 2025
View reviewed changes
Copy link
Member

@lnicola lnicola left a comment

Choose a reason for hiding this comment

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

Thanks! Will merge this tomorrow.

@BenjaminBrienen
Copy link
Contributor Author

Thanks! Will merge this tomorrow.

Gentle reminder

lnicola
lnicola reviewed Feb 26, 2025
View reviewed changes
@lnicola
Copy link
Member

lnicola commented Feb 26, 2025

PS: can you also squash this at the end?

@BenjaminBrienen
Copy link
Contributor Author

It doesn't get squashed when you merge it?

@lnicola
Copy link
Member

lnicola commented Feb 26, 2025

No, not when using merge queues.

@BenjaminBrienen BenjaminBrienen force-pushed the node branch 2 times, most recently from b86d7f6 to 5a02265 Compare February 26, 2025 16:10
@BenjaminBrienen
Copy link
Contributor Author

BenjaminBrienen commented Feb 26, 2025 •
edited
Loading

--experimental-strip-types doesn't exist on node 20, but I think we can target node20 from Node.js 22. Otherwise, the format and format:check scripts won't work (the old script or the new script).

@lnicola
Copy link
Member

lnicola commented Feb 26, 2025

Bah, can we switch to Biome or something? Not in this PR, of course.

@BenjaminBrienen
Copy link
Contributor Author

The squash is tasty

@lnicola lnicola enabled auto-merge February 26, 2025 16:19
auto-merge was automatically disabled February 27, 2025 19:53

Head branch was pushed to by a user without write access

@BenjaminBrienen
Copy link
Contributor Author

@lnicola I had to fix a merge conflict in release.yml

@lnicola lnicola enabled auto-merge February 27, 2025 20:05
@lnicola lnicola dismissed davidbarsky’s stale review February 27, 2025 21:24

Looks like we can't merge this directly, but the Code version was rolled back.

@lnicola lnicola added this pull request to the merge queue Feb 27, 2025
Merged via the queue into rust-lang:master with commit fe3eda7 Feb 27, 2025
9 checks passed
@BenjaminBrienen BenjaminBrienen deleted the node branch February 27, 2025 21:48
@NickAcPT NickAcPT mentioned this pull request Mar 2, 2025
Closed
Veykril
Veykril reviewed Mar 2, 2025
View reviewed changes
editors/code/src/diagnostics.ts
Comment on lines -165 to +178
"ansi-black": "ansiBlack",
"ansi-white": "ansiWhite",
"ansi-red": "ansiRed",
"ansi-green": "ansiGreen",
"ansi-yellow": "ansiYellow",
"ansi-blue": "ansiBlue",
"ansi-magenta": "ansiMagenta",
"ansi-cyan": "ansiCyan",

"ansi-bright-black": "ansiBrightBlack",
"ansi-bright-white": "ansiBrightWhite",
"ansi-bright-red": "ansiBrightRed",
"ansi-bright-green": "ansiBrightGreen",
"ansi-bright-yellow": "ansiBrightYellow",
"ansi-bright-blue": "ansiBrightBlue",
"ansi-bright-magenta": "ansiBrightMagenta",
"ansi-bright-cyan": "ansiBrightCyan",
"ansi-black": new ThemeColor("terminal.ansiBlack"),
"ansi-white": new ThemeColor("terminal.ansiWhite"),
"ansi-red": new ThemeColor("terminal.ansiRed"),
"ansi-green": new ThemeColor("terminal.ansiGreen"),
"ansi-yellow": new ThemeColor("terminal.ansiYellow"),
"ansi-blue": new ThemeColor("terminal.ansiBlue"),
"ansi-magenta": new ThemeColor("terminal.ansiMagenta"),
"ansi-cyan": new ThemeColor("terminal.ansiCyan"),

"ansi-bright-black": new ThemeColor("terminal.ansiBrightBlack"),
"ansi-bright-white": new ThemeColor("terminal.ansiBrightWhite"),
"ansi-bright-red": new ThemeColor("terminal.ansiBrightRed"),
"ansi-bright-green": new ThemeColor("terminal.ansiBrightGreen"),
"ansi-bright-yellow": new ThemeColor("terminal.ansiBrightYellow"),
"ansi-bright-blue": new ThemeColor("terminal.ansiBrightBlue"),
"ansi-bright-magenta": new ThemeColor("terminal.ansiBrightMagenta"),
"ansi-bright-cyan": new ThemeColor("terminal.ansiBrightCyan"),
Copy link
Member

@Veykril Veykril Mar 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

As pointed out by #19262 this change is wrong / incomplete. The type hints may have lied but the code using the static was expecting strings as values, not ThemeColor objects

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Veykril Veykril Veykril left review comments

@lnicola lnicola lnicola approved these changes

@davidbarsky davidbarsky davidbarsky left review comments

Assignees
No one assigned
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@BenjaminBrienen @lnicola @davidbarsky @Veykril @rustbot