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

watch: watch for unexsiting dependencies #45348

Merged
merged 5 commits into from Nov 13, 2022

Conversation

MoLow
Copy link
Member

@MoLow MoLow commented Nov 6, 2022

an alternative for #45259
if we choose to favor this solution, need to add some tests

@MoLow MoLow added the watch-mode Issues and PRs related to watch mode label Nov 6, 2022
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels Nov 6, 2022
@MoLow
Copy link
Member Author

MoLow commented Nov 6, 2022

CC @ruyadorno

@@ -254,6 +254,9 @@ function finalizeResolution(resolved, base, preserveSymlinks) {
err.url = String(resolved);
throw err;
} else if (!stats.isFile()) {
if (process.env.WATCH_REPORT_DEPENDENCIES && process.send) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we prefix all our environment variables with NODE_: https://nodejs.org/api/cli.html#environment-variables

Copy link
Member Author

Choose a reason for hiding this comment

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

Will fix in a follow up PR, as this environment variable existed before this pr

Copy link
Member

@ruyadorno ruyadorno left a comment

Choose a reason for hiding this comment

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

looks good 👍 thanks @MoLow!

test/sequential/test-watch-mode.mjs Show resolved Hide resolved
@ruyadorno
Copy link
Member

@MoLow also, there's a typo on your original commit, it should pb say watch for missing dependencies instead

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Can you explain how this fixes the issue? Why is telling watch mode that loading a module failed fix things? Is is because those files are removed so they can be re-added later when they are re-generated?

@MoLow
Copy link
Member Author

MoLow commented Nov 12, 2022

Is is because those files are removed so they can be re-added later when they are re-generated?

that is the precise reason. see the tests added in the original PR

it('should watch changes to previously loaded dependencies', async () => {

@MoLow MoLow added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Nov 12, 2022
@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 13, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 13, 2022
@nodejs-github-bot
Copy link
Collaborator

@MoLow MoLow mentioned this pull request Nov 13, 2022
4 tasks
@MoLow MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 13, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 13, 2022
@nodejs-github-bot nodejs-github-bot merged commit ce16bf5 into nodejs:main Nov 13, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in ce16bf5

@MoLow MoLow deleted the watch-reload-not-found branch November 13, 2022 15:14
ruyadorno pushed a commit that referenced this pull request Nov 21, 2022
PR-URL: #45348
Reviewed-By: Ruy Adorno <ruyadorno@google.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
MoLow added a commit to MoLow/node that referenced this pull request Nov 24, 2022
PR-URL: nodejs#45348
Reviewed-By: Ruy Adorno <ruyadorno@google.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
MoLow added a commit to MoLow/node that referenced this pull request Nov 24, 2022
PR-URL: nodejs#45348
Reviewed-By: Ruy Adorno <ruyadorno@google.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Nov 24, 2022
MoLow added a commit to MoLow/node that referenced this pull request Dec 9, 2022
PR-URL: nodejs#45348
Reviewed-By: Ruy Adorno <ruyadorno@google.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #45348
Reviewed-By: Ruy Adorno <ruyadorno@google.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #45348
Reviewed-By: Ruy Adorno <ruyadorno@google.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
PR-URL: #45348
Reviewed-By: Ruy Adorno <ruyadorno@google.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
PR-URL: #45348
Reviewed-By: Ruy Adorno <ruyadorno@google.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. needs-ci PRs that need a full CI run. watch-mode Issues and PRs related to watch mode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants