Skip to content

Migrate extension to node 18 - #21937

Closed
Karthik Nadig (karthiknadig) wants to merge 5 commits into
microsoft:mainfrom
karthiknadig:node18
Closed

Migrate extension to node 18#21937
Karthik Nadig (karthiknadig) wants to merge 5 commits into
microsoft:mainfrom
karthiknadig:node18

Conversation

@karthiknadig

Copy link
Copy Markdown
Member

No description provided.

@karrtikr Kartik Raj (karrtikr) left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Are you able to reproduce this issue locally? Try running npm run compile inside the pythonExtensionApi directory.

@karthiknadig

Karthik Nadig (karthiknadig) commented Sep 6, 2023

Copy link
Copy Markdown
Member Author

Compile passes but this still fails locally:

image

@karrtikr Kartik Raj (karrtikr) self-assigned this Sep 7, 2023

@karrtikr Kartik Raj (karrtikr) left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Compile passes

I assume this was run inside the api directory.

Can we also upgrade the node used by devcontainers/codespaces? cc/ Anthony Kim (@anthonykim1)

@karrtikr Kartik Raj (karrtikr) removed their assignment Sep 7, 2023
@anthonykim1

Anthony Kim (anthonykim1) commented Sep 7, 2023

Copy link
Copy Markdown

Can we also upgrade the node used by devcontainers/codespaces? cc/ Anthony Kim (@anthonykim1)

Yes! That should be totally possible Kartik Raj (@karrtikr)
I can take care of that. #21949

@karthiknadig

Copy link
Copy Markdown
Member Author

Compile passes

I assume this was run inside the api directory.

Yes it was run in the API directory.

@karrtikr

Copy link
Copy Markdown

Karthik Nadig (@karthiknadig) Error is coming from a package https://www.npmjs.com/package/gulp-typescript which hasn't been updated in 4 years, so likely isn't adaptive to Node18.

Is there a way to make the package use our "source-map" dependency instead of its own node_modules/gulp-typescript/node_modules/source-map?

@karthiknadig

Copy link
Copy Markdown
Member Author

Can we just drop gulp? Use nox as task runner.

Kartik Raj added 2 commits September 7, 2023 22:40
@karrtikr

Copy link
Copy Markdown

Karthik Nadig (@karthiknadig) #21937 (comment) should be fixed now.

@karrtikr

Copy link
Copy Markdown

Can we just drop gulp? Use nox as task runner.

Except for one package

image

I think we can find nox-equivalent for most other stuff.

@karrtikr Kartik Raj (karrtikr) left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We should also create/link an issue corresponding to this PR.

@karrtikr
Kartik Raj (karrtikr) removed their request for review September 7, 2023 23:25
@karthiknadig

Copy link
Copy Markdown
Member Author

We might need to update the test runner. Seems to be failing on windows

@karthiknadig

Copy link
Copy Markdown
Member Author

Karthik Nadig (@karthiknadig) I think we can move everything over to nox. See here for implementation of node-has-native-dependencies. It is just looking for files by name binding.gyp: https://github.com/sramam/node-has-native-dependencies/blob/master/index.js

@karrtikr

Copy link
Copy Markdown

Did not have permission to rebase and force push on this branch hence created a new PR: #22135.

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

Labels

debt Code quality issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants