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

watch: watch for unexsiting dependencies by MoLow · Pull Request #45348 · nodejs/node · GitHub

/ node Public

watch: watch for unexsiting dependencies - #45348

Merged
nodejs-github-bot merged 5 commits into
nodejs:mainfrom
MoLow:watch-reload-not-found
Nov 13, 2022
ConversationCommits (5)ChecksFiles changed
Merged

watch: watch for unexsiting dependencies#45348
nodejs-github-bot merged 5 commits into
nodejs:mainfrom
MoLow:watch-reload-not-found

Conversation

MoLow commented Nov 6, 2022

Copy link
Copy Markdown
Member

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

MoLow added the watch-mode Issues and PRs related to watch mode label Nov 6, 2022

nodejs-github-bot commented Nov 6, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/modules

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 commented Nov 6, 2022

Copy link
Copy Markdown
Member Author

CC @ruyadorno

GeoffreyBooth reviewed Nov 6, 2022
View reviewed changes
Comment thread lib/internal/modules/esm/resolve.js Outdated

GeoffreyBooth Nov 6, 2022

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 we prefix all our environment variables with NODE_: https://nodejs.org/api/cli.html#environment-variables

MoLow Nov 7, 2022

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

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

ruyadorno reviewed Nov 7, 2022
View reviewed changes

ruyadorno 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

looks good 👍 thanks @MoLow!

Comment thread test/sequential/test-watch-mode.mjs Outdated
ruyadorno mentioned this pull request Nov 7, 2022
Closed

ruyadorno commented Nov 9, 2022

Copy link
Copy Markdown
Member

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

MoLow force-pushed the watch-reload-not-found branch from d686854 to 1d02416 Compare November 10, 2022 11:54
MoLow force-pushed the watch-reload-not-found branch from 1d02416 to 56ec098 Compare November 10, 2022 15:48
ljharb reviewed Nov 10, 2022
View reviewed changes
Comment thread test/sequential/test-watch-mode.mjs Outdated
benjamingr reviewed Nov 11, 2022
View reviewed changes
Comment thread test/sequential/test-watch-mode.mjs Outdated
benjamingr reviewed Nov 11, 2022
View reviewed changes
Comment thread test/sequential/test-watch-mode.mjs Outdated
benjamingr reviewed Nov 11, 2022
View reviewed changes
Comment thread test/sequential/test-watch-mode.mjs Outdated
benjamingr reviewed Nov 11, 2022
View reviewed changes

benjamingr 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

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 commented Nov 12, 2022

Copy link
Copy Markdown
Member Author

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 force-pushed the watch-reload-not-found branch from e9b0848 to d1c155c Compare November 12, 2022 18:40
MoLow requested a review from benjamingr November 12, 2022 18:41
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 added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 13, 2022
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 commented Nov 13, 2022

Copy link
Copy Markdown
Collaborator

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

MoLow mentioned this pull request Nov 13, 2022
Merged
4 tasks
MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 13, 2022
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 merged commit ce16bf5 into nodejs:main Nov 13, 2022

nodejs-github-bot commented Nov 13, 2022

Copy link
Copy Markdown
Collaborator

Landed in ce16bf5

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
watch: watch for missing dependencies
3263ceb 
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 mentioned this pull request Nov 24, 2022
Closed
ruyadorno mentioned this pull request Nov 24, 2022
Merged
MoLow added a commit to MoLow/node that referenced this pull request Dec 9, 2022
watch: watch for missing dependencies
a47c61e 
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>
merceyz mentioned this pull request Dec 9, 2022
Merged
3 tasks
danielleadams mentioned this pull request Dec 30, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
watch: watch for missing dependencies
b16807a 
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
watch: watch for missing dependencies
24bfe54 
PR-URL: #45348
Reviewed-By: Ruy Adorno <ruyadorno@google.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
merceyz mentioned this pull request Jan 7, 2023
Merged
3 tasks
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

ljharb ljharb left review comments
GeoffreyBooth GeoffreyBooth left review comments
ruyadorno ruyadorno approved these changes
benjamingr benjamingr approved these changes
+1 more reviewer
Linkgoron Linkgoron approved these changes
Reviewers whose approvals may not affect merge requirements

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

Back | FazBrowse Home | New Git URL