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

Migrate extension to node 18 #21937

Closed
wants to merge 5 commits into from
Closed

Conversation

karthiknadig
Copy link
Member

No description provided.

@karthiknadig karthiknadig self-assigned this Sep 6, 2023
@karthiknadig karthiknadig added the debt Covers everything internal: CI, testing, refactoring of the codebase, etc. label Sep 6, 2023
Copy link

@karrtikr karrtikr left a comment

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
Copy link
Member Author

karthiknadig commented Sep 6, 2023

Compile passes but this still fails locally:

image

@karrtikr karrtikr self-assigned this Sep 7, 2023
Copy link

@karrtikr karrtikr left a comment

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/ @anthonykim1

@karrtikr karrtikr removed their assignment Sep 7, 2023
@anthonykim1
Copy link

anthonykim1 commented Sep 7, 2023

Can we also upgrade the node used by devcontainers/codespaces? cc/ @anthonykim1

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

@karthiknadig
Copy link
Member Author

Compile passes

I assume this was run inside the api directory.

Yes it was run in the API directory.

@karrtikr
Copy link

karrtikr commented Sep 7, 2023

@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
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

karrtikr commented Sep 7, 2023

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

@karrtikr
Copy link

karrtikr commented Sep 7, 2023

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.

Copy link

@karrtikr karrtikr left a comment

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.

@anthonykim1 anthonykim1 requested a review from karrtikr September 7, 2023 23:12
@karrtikr karrtikr removed their request for review September 7, 2023 23:25
@karthiknadig
Copy link
Member Author

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

@karthiknadig
Copy link
Member Author

@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

karrtikr commented Oct 2, 2023

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

@karrtikr karrtikr closed this Oct 2, 2023
@karthiknadig karthiknadig deleted the node18 branch October 4, 2023 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debt Covers everything internal: CI, testing, refactoring of the codebase, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants