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

watch: watch for unexsiting dependencies #45348

Merged
merged 5 commits into from
Nov 13, 2022
Merged

watch: watch for unexsiting dependencies #45348

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

GeoffreyBooth
GeoffreyBooth reviewed Nov 6, 2022
View reviewed changes
lib/internal/modules/esm/resolve.js
@@ -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

ruyadorno
ruyadorno reviewed Nov 7, 2022
View reviewed changes
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 ruyadorno mentioned this pull request Nov 7, 2022
Closed
@ruyadorno
Copy link
Member

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

@MoLow MoLow force-pushed the watch-reload-not-found branch from d686854 to 1d02416 Compare November 10, 2022 11:54
@MoLow MoLow force-pushed the watch-reload-not-found branch from 1d02416 to 56ec098 Compare November 10, 2022 15:48
benjamingr
benjamingr reviewed Nov 11, 2022
View reviewed changes
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

node/test/sequential/test-watch-mode.mjs

Line 178 in df6b0cd

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

@MoLow MoLow force-pushed the watch-reload-not-found branch from e9b0848 to d1c155c Compare November 12, 2022 18:40
@MoLow MoLow requested a review from benjamingr November 12, 2022 18:41
@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

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

@MoLow MoLow mentioned this pull request Nov 13, 2022
Merged
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
@MoLow @ruyadorno
watch: watch for missing dependencies
3263ceb 
PR-URL: #45348
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Nitzan Uziely <[email protected]>
MoLow added a commit to MoLow/node that referenced this pull request Nov 24, 2022
@MoLow
watch: watch for missing dependencies
001cd55 
PR-URL: nodejs#45348
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Nitzan Uziely <[email protected]>
MoLow added a commit to MoLow/node that referenced this pull request Nov 24, 2022
@MoLow
watch: watch for missing dependencies
176d431 
PR-URL: nodejs#45348
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Nitzan Uziely <[email protected]>
@MoLow MoLow mentioned this pull request Nov 24, 2022
Closed
@ruyadorno ruyadorno mentioned this pull request Nov 24, 2022
Merged
MoLow added a commit to MoLow/node that referenced this pull request Dec 9, 2022
@MoLow
watch: watch for missing dependencies
a47c61e 
PR-URL: nodejs#45348
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Nitzan Uziely <[email protected]>
@merceyz merceyz mentioned this pull request Dec 9, 2022
Merged
3 tasks
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@MoLow @danielleadams
watch: watch for missing dependencies
5e34be5 
PR-URL: #45348
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Nitzan Uziely <[email protected]>
@danielleadams danielleadams mentioned this pull request Dec 30, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@MoLow @danielleadams
watch: watch for missing dependencies
b16807a 
PR-URL: #45348
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Nitzan Uziely <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
@MoLow @danielleadams
watch: watch for missing dependencies
24bfe54 
PR-URL: #45348
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Nitzan Uziely <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
@MoLow @danielleadams
watch: watch for missing dependencies
43e3eaa 
PR-URL: #45348
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Nitzan Uziely <[email protected]>
@merceyz merceyz mentioned this pull request Jan 7, 2023
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GeoffreyBooth GeoffreyBooth GeoffreyBooth left review comments

@ljharb ljharb ljharb left review comments

@benjamingr benjamingr benjamingr approved these changes

@Linkgoron Linkgoron Linkgoron approved these changes

@ruyadorno ruyadorno ruyadorno approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@MoLow @nodejs-github-bot @ruyadorno @ljharb @GeoffreyBooth @Linkgoron @benjamingr