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 mode: Allow adding files to watch when using the --watch flag #45467

Closed
gustavnikolaj opened this issue Nov 15, 2022 · 13 comments · May be fixed by #46000
Closed

Watch mode: Allow adding files to watch when using the --watch flag #45467

gustavnikolaj opened this issue Nov 15, 2022 · 13 comments · May be fixed by #46000
Labels
feature request Issues that request new features to be added to Node.js. watch-mode Issues and PRs related to watch mode

Comments

@gustavnikolaj
Copy link

What is the problem this feature will solve?

The new --watch flag will automatically restart the process when any imported file is changed.

The new related --watch-path flag will restart the process when one of the added paths change.

Using --watch-path disables the automatic restarting based on imported modules.

--watch is what you want 99% of the time, but there are cases where you want to extend the watched files ever-so-slightly.

One such example is .env when used with the dotenv module. No matter if you do it programmatically, or through the use of a -r flag to the node process, changes in the .env. file will not trigger a reload.

Other examples could be configuration files in json, yaml og toml formats.

What is the feature you are proposing to solve the problem?

A new flag which will allow extending the set of watched files based on what is imported/required, with specific non-js files which are loaded through non-discoverable methods, without disabling the automatic discovery of files.

The automatic discoverability of files to watch is a killer feature! Unfortunately not being able to add individual files to the watched set is preventing me from using it.

What alternatives have you considered?

I can keep using nodemon or the current --watch-path feature, but have to enumerate and eagerly watch all files that could possibly be loaded. But as --watch-path does not work on Linux it will likely have to be nodemon.

@gustavnikolaj gustavnikolaj added the feature request Issues that request new features to be added to Node.js. label Nov 15, 2022
@gustavnikolaj
Copy link
Author

cc @MoLow

(I would add the watch-mode label if I had permission to do so)

@cola119 cola119 added the watch-mode Issues and PRs related to watch mode label Nov 15, 2022
@aduh95
Copy link
Contributor

aduh95 commented Nov 15, 2022

Could you illustrate your request with a minimal example? IIUC for the following setup:

echo '{"key": "initial value"}' > file.json
echo 'module.exports = JSON.parse(require("node:fs").readFileSync("./file.json", "utf-8"))' > dep.js
echo 'console.log(require("./dep.js"))' > entry.js
node --watch-path=file.json --watch entry.js

You would like that on top of any change to file.json, any change to dep.js would also restart the process, is that correct? Or are you talking about having a new API, and if so, can you give an example how it could look like?

@gustavnikolaj
Copy link
Author

You would like that on top of any change to file.json, any change to dep.js would also restart the process, is that correct?

That illustrates the behavior I'm looking for, correctly!

Some javascript code loads a non-javascript file and use it as input. In my example it is the dotenv package, which when used as node --require dotenv/config ... will load the .env file in the current working directory and populate process.env with the values there. I want changes in the .env file to cause a reload, and I am alright that I have to manually tell node to do so, when it cannot trivially be statically analyzed.

Or are you talking about having a new API, and if so, can you give an example how it could look like?

Honestly, I don't really know. The default behavior of --watch is brilliant. This is how nodemon or all other watch tool should work by default, and why such a tool belongs in core. I was a bit surprised to see that --watch-path implicitly de-activate the behavior of --watch, but I assume that there is a good reason.

--watch-path-but-also-do-not-disable-normal-watch is my naive request, but I am pretty sure that there is a better way. And I'm also pretty sure that I am not the only one with this use case, so I thought I would add the feedback here when I did not find another issue addressing it.

@MoLow MoLow added the good first issue Issues that are suitable for first-time contributors. label Nov 15, 2022
@MoLow
Copy link
Member

MoLow commented Nov 15, 2022

@gustavnikolaj are you interested in creating a PR to support your use-case?
we can either add a new flag for that, or make --watch-path behave the way it does only in case passed exclusively (i.e without --watch

@umutciloglu
Copy link

I'm not really experienced but i can take a look if it's ok.

@MoLow
Copy link
Member

MoLow commented Nov 23, 2022

Go for it!

@debadree25
Copy link
Member

Hello, could you guide me on any supporting documentation or relevant parts of the codebase I could look into?

@debadree25
Copy link
Member

Have been attempting to take a stab at this, still unable to find the place where the import logic is written, any pointers would be very helpful
cc @MoLow

Thank You!

@MoLow
Copy link
Member

MoLow commented Dec 4, 2022

@debadree25 most of the watch mode implementation is in this file: https://github.com/nodejs/node/blob/de696d70ef1d0a5ff52312709914134306bd7c82/lib/internal/main/watch_mode.js

@debadree25
Copy link
Member

I tried to run @aduh95 's example curiously I see an empty output, could be doing something wrong, or could this be a bug:
Screenshot 2022-12-05 at 8 32 47 PM
running node v18.11.0 on macOS 13.0.1

ydeshayes added a commit to ydeshayes/node that referenced this issue Dec 28, 2022
This patch is adding support for --watch-path when using --watch to
watch extra files/paths. When used together, the path(s) passed to --watch-path
will be added to the filteredFiles in the file watcher.

Fixes: nodejs#45467
@theoephraim
Copy link

It may be tricky depending on the implementation, but it could also make sense to let the code programatically add files to the watch list -- which could even reset on each restart to keep things simple.

But that way the file that is deciding where those config files are and their paths can trigger adding them rather than having to modify the way I run the program itself.

paescuj added a commit to directus/directus that referenced this issue Apr 4, 2023
Is a bit faster and we can rely on the built-in `watch` and `inspect`
functionalities of Node.js

Note: The possibility to watch other files (.env in our case) might be
added in the future, see nodejs/node#45467
rijkvanzanten added a commit to directus/directus that referenced this issue Apr 4, 2023
…g One (#18014)

* Step 1

* Step 2

* False sense of confidence

* Couple more before dinner

* Update schema package

* Update format-title

* Upgrade specs file

* Close

* Replace ts-node-dev with tsx, and various others

* Replace lodash with lodash-es

* Add lodash-es types

* Update knex import

* More fun is had

* FSE

* Consolidate repos

* Various tweaks and fixes

* Fix specs

* Remove dependency on knex-schema-inspector

* Fix wrong imports of inspector

* Move shared exceptions to new package

* Move constants to separate module

* Move types to new types package

* Use directus/types

* I believe this is no longer needed

* [WIP] Start moving utils to esm

* ESMify Shared

* Move shared utils to  @directus/utils

* Use @directus/utils instead of @directus/shared/utils

* It runs!

* Use correct schemaoverview type

* Fix imports

* Fix the thing

* Start on new update-checker lib

* Use new update-check package

* Swap out directus/shared in app

* Pushing through the last bits now

* Dangerously make extensions SDK ESM

* Use @directus/types in tests

* Copy util function to test

* Fix linter config

* Add missing import

* Hot takes

* Fix build

* Curse these default exports

* No tests in constants

* Add tests

* Remove tests from types

* Add tests for exceptions

* Fix test

* Fix app tests

* Fix import in test

* Fix various tests

* Fix specs export

* Some more tests

* Remove broken integration tests

These were broken beyond repair.. They were also written before we really knew what we we're doing with tests, so I think it's better to say goodbye and start over with these

* Regenerate lockfile

* Fix imports from merge

* I create my own problems

* Make sharp play nice

* Add vitest config

* Install missing blackbox dep

* Consts shouldn't be in types

tsk tsk tsk tsk

* Fix type/const usage in extensions-sdk

* cursed.default

* Reduce circular deps

* Fix circular dep in items service

* vvv

* Trigger testing for all vendors

* Add workaround for rollup

* Prepend the file protocol for the ESM loader to be compatible with Windows
"WARN: Only URLs with a scheme in: file and data are supported by the default ESM loader. On Windows, absolute paths must be valid file:// URLs. Received protocol 'c:'"

* Fix postgres

* Schema package updates

Co-authored-by: Azri Kahar <[email protected]>

* Resolve cjs/mjs extensions

* Clean-up eslint config

* fixed extension concatination

* using string interpolation for consistency

* Revert MySQL optimisation

* Revert testing for all vendors

* Replace tsx with esbuild-kit/esm-loader

Is a bit faster and we can rely on the built-in `watch` and `inspect`
functionalities of Node.js

Note: The possibility to watch other files (.env in our case) might be
added in the future, see nodejs/node#45467

* Use exact version for esbuild-kit/esm-loader

* Fix import

---------

Co-authored-by: ian <[email protected]>
Co-authored-by: Brainslug <[email protected]>
Co-authored-by: Azri Kahar <[email protected]>
Co-authored-by: Pascal Jufer <[email protected]>
@NwaforAugustine321
Copy link

NwaforAugustine321 commented May 12, 2023

@MoLow I want to know if the issue is still open for me to work on it. Thanks and am looking forward to hearing from you

meditadvisors pushed a commit to ciso360ai/directus-mod that referenced this issue May 13, 2023
…g One (directus#18014)

* Step 1

* Step 2

* False sense of confidence

* Couple more before dinner

* Update schema package

* Update format-title

* Upgrade specs file

* Close

* Replace ts-node-dev with tsx, and various others

* Replace lodash with lodash-es

* Add lodash-es types

* Update knex import

* More fun is had

* FSE

* Consolidate repos

* Various tweaks and fixes

* Fix specs

* Remove dependency on knex-schema-inspector

* Fix wrong imports of inspector

* Move shared exceptions to new package

* Move constants to separate module

* Move types to new types package

* Use directus/types

* I believe this is no longer needed

* [WIP] Start moving utils to esm

* ESMify Shared

* Move shared utils to  @directus/utils

* Use @directus/utils instead of @directus/shared/utils

* It runs!

* Use correct schemaoverview type

* Fix imports

* Fix the thing

* Start on new update-checker lib

* Use new update-check package

* Swap out directus/shared in app

* Pushing through the last bits now

* Dangerously make extensions SDK ESM

* Use @directus/types in tests

* Copy util function to test

* Fix linter config

* Add missing import

* Hot takes

* Fix build

* Curse these default exports

* No tests in constants

* Add tests

* Remove tests from types

* Add tests for exceptions

* Fix test

* Fix app tests

* Fix import in test

* Fix various tests

* Fix specs export

* Some more tests

* Remove broken integration tests

These were broken beyond repair.. They were also written before we really knew what we we're doing with tests, so I think it's better to say goodbye and start over with these

* Regenerate lockfile

* Fix imports from merge

* I create my own problems

* Make sharp play nice

* Add vitest config

* Install missing blackbox dep

* Consts shouldn't be in types

tsk tsk tsk tsk

* Fix type/const usage in extensions-sdk

* cursed.default

* Reduce circular deps

* Fix circular dep in items service

* vvv

* Trigger testing for all vendors

* Add workaround for rollup

* Prepend the file protocol for the ESM loader to be compatible with Windows
"WARN: Only URLs with a scheme in: file and data are supported by the default ESM loader. On Windows, absolute paths must be valid file:// URLs. Received protocol 'c:'"

* Fix postgres

* Schema package updates

Co-authored-by: Azri Kahar <[email protected]>

* Resolve cjs/mjs extensions

* Clean-up eslint config

* fixed extension concatination

* using string interpolation for consistency

* Revert MySQL optimisation

* Revert testing for all vendors

* Replace tsx with esbuild-kit/esm-loader

Is a bit faster and we can rely on the built-in `watch` and `inspect`
functionalities of Node.js

Note: The possibility to watch other files (.env in our case) might be
added in the future, see nodejs/node#45467

* Use exact version for esbuild-kit/esm-loader

* Fix import

---------

Co-authored-by: ian <[email protected]>
Co-authored-by: Brainslug <[email protected]>
Co-authored-by: Azri Kahar <[email protected]>
Co-authored-by: Pascal Jufer <[email protected]>
@MoLow MoLow removed the good first issue Issues that are suitable for first-time contributors. label Jul 26, 2023
@MoLow
Copy link
Member

MoLow commented Jul 26, 2023

I am closing this issue - I think #45182 will provide a better solution for this and is now easier to implement

@MoLow MoLow closed this as not planned Won't fix, can't repro, duplicate, stale Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. watch-mode Issues and PRs related to watch mode
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants