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

[dart2js] Fix compatiblity with Node.js 21 #53796

Closed
wants to merge 7 commits into from
Closed

[dart2js] Fix compatiblity with Node.js 21 #53796

wants to merge 7 commits into from

Conversation

ntkme
Copy link
Contributor

@ntkme ntkme commented Oct 18, 2023

This PR fixes #53784.

@copybara-service
Copy link

Thank you for your contribution! This project uses Gerrit for code reviews. Your pull request has automatically been converted into a code review at:

https://dart-review.googlesource.com/c/sdk/+/330987

Please wait for a developer to review your code review at the above link; you can speed up the review if you sign into Gerrit and manually add a reviewer that has recently worked on the relevant code. See CONTRIBUTING.md to learn how to upload changes to Gerrit directly.

Additional commits pushed to this PR will update both the PR and the corresponding Gerrit CL. After the review is complete on the CL, your reviewer will merge the CL (automatically closing this PR).

@sigmundch
Copy link
Member

Thanks so much for the contribution, added @rakudrama on the gerrit CL for review.

@ntkme
Copy link
Contributor Author

ntkme commented Oct 18, 2023

There are a few other places in this repo where generated codes need to be updated. I'm not sure what's the right way to get each of those updated. Please advice.

@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/330987 has been updated with the latest commits from this pull request.

1 similar comment
@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/330987 has been updated with the latest commits from this pull request.

@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/330987 has been updated with the latest commits from this pull request.

1 similar comment
@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/330987 has been updated with the latest commits from this pull request.

@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/330987 has been updated with the latest commits from this pull request.

1 similar comment
@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/330987 has been updated with the latest commits from this pull request.

@rakudrama
Copy link
Member

There are a few other places in this repo where generated codes need to be updated. I'm not sure what's the right way to get each of those updated. Please advice.

Can you explain where these places are, e.g. an example of some generated code, or a location in the repo?

@ntkme
Copy link
Contributor Author

ntkme commented Oct 18, 2023 •
edited
Loading

@rakudrama pkg/dds/test/web/sse_smoke_driver.dart.js and pkg/compiler/test/dump_info/data/js_members.dart contains the generated code.

@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/330987 has been updated with the latest commits from this pull request.

1 similar comment
@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/330987 has been updated with the latest commits from this pull request.

@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/330987 has been updated with the latest commits from this pull request.

1 similar comment
@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/330987 has been updated with the latest commits from this pull request.

@ntkme
Copy link
Contributor Author

ntkme commented Oct 18, 2023 •
edited
Loading

@rakudrama The latest commit should address your concerns in review. Other generated codes need updates are all tests, so they can probably be updated later as long as the sdk repo itself is not being tested with node 21.

As for how this changed tested, I'm testing it with dart-sass, which is what most of the users are complaining about. I first ran dart-sass' test with node 21 and official dart-sdk 3.1.4 and confirm it fails. Then I build a custom 3.1.4 with this patch applied on top, and ran dart-sass' test again, everything seems to be fine.

@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/330987 has been updated with the latest commits from this pull request.

1 similar comment
@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/330987 has been updated with the latest commits from this pull request.

@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/330987 has been updated with the latest commits from this pull request.

1 similar comment
@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/330987 has been updated with the latest commits from this pull request.

@ntkme
Copy link
Contributor Author

ntkme commented Oct 18, 2023

Looks like tests in pkg/compiler/test/dump_info failed due to outdated generated code, but I'm not sure how these are supposed to be updated.

@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/330987 has been updated with the latest commits from this pull request.

1 similar comment
@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/330987 has been updated with the latest commits from this pull request.

@ntkme
Copy link
Contributor Author

ntkme commented Oct 18, 2023 •
edited
Loading

@rakudrama I updated the dump_info with newly built sdk and now the failed test passes for me locally. Would you please retry it on CI?

@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/330987 has been updated with the latest commits from this pull request.

1 similar comment
@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/330987 has been updated with the latest commits from this pull request.

@ntkme
Copy link
Contributor Author

ntkme commented Oct 19, 2023

@rakudrama Sorry that I'm very not familiar with gerrit and its flow. It looks like the tests have all passed but gerrit dismissed a previous vote because I merged latest main into this feature branch after the vote. I'm not sure what action is needed from here. Please let me know if anything is needed.

@rakudrama
Copy link
Member

I think it looks good.
Let me run the tests again, and commit it from Gerrit if everything looks ok.
Don't push anything unless I say a fix is needed. It is kind of annoying how changes halt the tests and cancel votes.

Thanks for your help and patience!

@ntkme ntkme deleted the node-navigator branch October 19, 2023 03:53
copybara-service bot pushed a commit that referenced this pull request Oct 21, 2023
@sigmundch
[stable] [dart2js] Fix compatiblity with Node.js 21
8fdf766 
Closes #53796

GitOrigin-RevId: caeeb52
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/330987
Cherry-pick-request: TBA
Change-Id: I459da4b14b29bccca5be2d053d472770f20b04a8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/331323
Reviewed-by: Stephen Adams <sra@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Node v21 - issue with navigator object
3 participants
@ntkme @sigmundch @rakudrama