FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: refactor to avoid mutation of global by a loader by targos · Pull Request #46220 · nodejs/node · GitHub

/ node Public

test: refactor to avoid mutation of global by a loader - #46220

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
targos:fix-resolve-type
Jan 19, 2023
ConversationCommits (2)ChecksFiles changed
Merged

test: refactor to avoid mutation of global by a loader#46220
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
targos:fix-resolve-type

Conversation

targos commented Jan 15, 2023

Copy link
Copy Markdown
Member

This makes the test compatible with off-thread loaders.

/cc @GeoffreyBooth @aduh95

nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jan 15, 2023
targos added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 15, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 15, 2023

nodejs-github-bot commented Jan 15, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/48991/

targos commented Jan 15, 2023
View reviewed changes
Comment thread test/fixtures/es-module-loaders/hook-resolve-type.mjs Outdated
Comment on lines 17 to 22

targos Jan 15, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

/cc @joyeecheung this is related to https://bugs.chromium.org/p/v8/issues/detail?id=13238 / #44409

aduh95 Jan 15, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

FWIW the current implementation of #44710 relies on Atomics.waitAsync not keeping the event loop alive.

targos Jan 16, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@aduh95 What would be your expectation? Should Atomics.waitAsync keep the event loop alive?

aduh95 Jan 16, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ideally there would be a way to control it, like you can opt-out for setTimeout. My expectation was that it would not keep the event loop alive, but I could see how that can also be annoying for some (most?) use cases. Does it also not keep the event loop alive if you add a fourth timeout parameter?

targos Jan 16, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The fourth timeout parameter doesn't change the behavior.

joyeecheung Jan 18, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think so far the vibes I got is that "this is host-defined" i.e. Node.js gets to decide if the timeout parameter keeps the event loop alive (neither the ES or the Web specs have this "shut down the loop on completion" concept, they just assume the agent runs forever until it gets told to shut down, so we are on our own here), but also if we want to implement that, V8 needs to provide a way in the platform API for canceling a delayed task (so that when it wakes up before the timeout, we get notified that there is no need to keep the event loop alive for it anymore), which is probably too much complexity, so to be lazy we can also just keep the things the way they are (not let the time out keep the event loop alive, which some may argue is also their expected behavior, like how an unresolved promise doesn't keep the thread alive).

GeoffreyBooth approved these changes Jan 15, 2023
View reviewed changes

GeoffreyBooth left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM. FYI in nodejs/loaders#124 (reply in thread) I’m discussing redesigning globalPreload, possibly by creating a new API to handle the communications port stuff and getting rid of the “sloppy mode script” aspect of globalPreload now that we have --import. I was thinking that that could come after we land off-thread, though, and possibly even after making loaders stable (I would keep globalPreload as experimental but mark resolve and load as stable).

GeoffreyBooth removed the needs-ci PRs that need a full CI run. label Jan 15, 2023

nodejs-github-bot commented Jan 15, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/48993/

GeoffreyBooth added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jan 15, 2023

GeoffreyBooth commented Jan 15, 2023

Copy link
Copy Markdown
Member

@targos I assume this new version of this test passes both on main and in the off-thread branch?

GeoffreyBooth mentioned this pull request Jan 15, 2023
Merged
7 tasks
test: refactor to avoid mutation of global by a loader
825261e 
This makes the test compatible with off-thread loaders.

Co-Authored-By: Geoffrey Booth <webadmin@geoffreybooth.com>

targos commented Jan 16, 2023

Copy link
Copy Markdown
Member Author

@GeoffreyBooth Yes, it passes on both branches.

GeoffreyBooth added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 16, 2023
nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 17, 2023

nodejs-github-bot commented Jan 17, 2023

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/46220
✔  Done loading data for nodejs/node/pull/46220
----------------------------------- PR info ------------------------------------
Title      test: refactor to avoid mutation of global by a loader (#46220)
Author     Michaël Zasso  (@targos)
Branch     targos:fix-resolve-type -> nodejs:main
Labels     test, esm, author ready
Commits    1
 - test: refactor to avoid mutation of global by a loader
Committers 1
 - Michaël Zasso 
PR-URL: https://github.com/nodejs/node/pull/46220
Reviewed-By: Antoine du Hamel 
Reviewed-By: Geoffrey Booth 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/46220
Reviewed-By: Antoine du Hamel 
Reviewed-By: Geoffrey Booth 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sun, 15 Jan 2023 12:07:37 GMT
   ✔  Approvals: 2
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/46220#pullrequestreview-1249340907
   ✔  - Geoffrey Booth (@GeoffreyBooth) (TSC): https://github.com/nodejs/node/pull/46220#pullrequestreview-1249358333
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-01-15T21:13:00Z: https://ci.nodejs.org/job/node-test-pull-request/48993/
   ⚠  Commits were pushed after the last Full PR CI run:
   ⚠  - test: refactor to avoid mutation of global by a loader
- Querying data for job/node-test-pull-request/48993/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/3939092857

aduh95 removed author ready PRs that have at least one approval, no outstanding review comments, and a CI started. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jan 18, 2023
Update test/fixtures/es-module-loaders/hook-resolve-type.mjs
de3d1fd 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Jan 18, 2023
aduh95 added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jan 18, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 18, 2023

nodejs-github-bot commented Jan 18, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/49059/

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 19, 2023
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 19, 2023
nodejs-github-bot merged commit cf8c699 into nodejs:main Jan 19, 2023

nodejs-github-bot commented Jan 19, 2023

Copy link
Copy Markdown
Collaborator

Landed in cf8c699

RafaelGSS pushed a commit that referenced this pull request Jan 20, 2023
test: refactor to avoid mutation of global by a loader
4ac5c71 
This makes the test compatible with off-thread loaders.

Co-Authored-By: Geoffrey Booth <webadmin@geoffreybooth.com>
PR-URL: #46220
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
RafaelGSS mentioned this pull request Jan 20, 2023
Merged
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
test: refactor to avoid mutation of global by a loader
2e0f047 
This makes the test compatible with off-thread loaders.

Co-Authored-By: Geoffrey Booth <webadmin@geoffreybooth.com>
PR-URL: #46220
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
juanarbol mentioned this pull request Jan 28, 2023
Merged
GeoffreyBooth mentioned this pull request Jan 29, 2023
Merged
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
test: refactor to avoid mutation of global by a loader
a3056f4 
This makes the test compatible with off-thread loaders.

Co-Authored-By: Geoffrey Booth <webadmin@geoffreybooth.com>
PR-URL: #46220
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

joyeecheung joyeecheung left review comments
aduh95 aduh95 approved these changes
GeoffreyBooth GeoffreyBooth approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. esm Issues and PRs related to the ECMAScript Modules implementation. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL