node icon indicating copy to clipboard operation
node copied to clipboard

nodemon (or similar) in core

Open bnb opened this issue 2 years ago • 37 comments

Is your feature request related to a problem? Please describe. Not particularly. It's related to a problem in that it is proposing the addition or inclusion of a tool that is widely used for a common problem.

Describe the solution you'd like

I recently put out a tweet from @nodejs on Twitter asking folks what tool they find most useful day-to-day. I generally try to do this not with the intent to bring a proposal or something like that, but to have some engaging activity that could be helpful to someone somewhere.

One thing I noticed was just how many people replied with "nodemon". It seemed to come up just as often as DevTools, VS Code, and TypeScript which I found remarkable.

I'd like to recommend that we consider either vendoring nodemon or explore writing a nodemon-like from scratch in core.

Describe alternatives you've considered Doing nothing, which would be fine. This is a problem that is obviously solvable in userland, but one that people clearly need solved. If the result of this issue is "this should be in userland" that's totally fine.

bnb avatar Oct 12 '21 17:10 bnb

I very much prefer https://github.com/fgnass/node-dev to nodemon mostly because nodemon restarts the whole process when it detects a change, whereas node-dev only reloads the single module which changed and it's dependents. On bigger projects this can save quite a lot of overhead.

capaj avatar Oct 12 '21 17:10 capaj

@capaj that's a different use case

boneskull avatar Oct 12 '21 17:10 boneskull

@boneskull why do you think it's a different usecase? I use it on typical backend projects interchangeably.

capaj avatar Oct 12 '21 18:10 capaj

it appears to be for development. but regardless, this issue is about nodemon, which many people use

boneskull avatar Oct 12 '21 18:10 boneskull

Are you talking about using nodemon as a tool to restart your server in production?

FYI most people use nodemon just for development. A very tiny minority of users use nodemon for other usecases. Also this issue is not about nodemon specifically. I understand it as a feature request for a tool to manage code reload for development of long running node.js processes. I think the author only mentioned nodemon because that is the most commonly used tool for this.

capaj avatar Oct 12 '21 18:10 capaj

I would like to start that we should look at solving the major use cases, not all possible edge cases with this. I do want this feature very badly.

  1. This would be ideal to enable the reload devtools workflows that are missing from node's devtools integration.
  2. This could enable various sane things like file watching causing restarts during development. Though, this could have some loss of data if done too pro-actively so likely needs some kind of config around how to do restarts.
  3. This could enable a sane path towards supporting import.meta.hot which isn't possible due to ESM cache having no way to safely eject entries in certain scenarios (effect of the ESM spec design).
  4. For "production" I think there are plenty of people using restart managers (nodemon or other) and any restart feature could be used for this purpose to avoid some problems in particular with people wanting to setup Windows Services that restart on exit (staring blankly at my minecraft server in the corner here).

I think determining what restart means (soft vs hard) would be a good first step to figuring out what could be in core. I think if we do want devtools integration that relies on reloads like perf tab to properly work we likely won't be able to just vendor in an existing library though. Also, node can likely spin up a much smaller footprint manager than a full blown JS VM.

I don't think bickering about gritty details and if something is the best solution needs to be done at the investigation phase of this. I think there will generally be more specialized solutions that people can use if whatever node ships doesn't fit their specific need. We are trying to help users and if we don't ship anything due to bickering, we don't improve the situation for anyone.

bmeck avatar Oct 12 '21 20:10 bmeck

I think there will generally be more specialized solutions that people can use if whatever node ships doesn't fit their specific need.

This feature request seems like one of those things that can be very opinionated, which makes me lean towards -1 for inclusion in node core. However, I suppose at the very least if it's possible to build/install node without whatever tool, I would be more ok with it. I already use (other) 3rd party tools for managing node processes and don't want to add more bloat to my node installations.

mscdex avatar Oct 12 '21 22:10 mscdex

@mscdex I'm not sure preventing every kilobyte of unused core is really a goal of Node. Providing use cases to users is paramount and various things like existing devtools workflows that are currently disabled in the node inspector need some kind of internal reload manager. We for example ship acorn which is not used outside of the REPL. We also have websockets implementation for the inspector but never expose it either. Fighting to avoid adding any features that won't be used in all cases is tantamount to ripping out most of core so node is not usable on its own and all features must be bespoke and from the ecosystem.

bmeck avatar Oct 13 '21 14:10 bmeck

We for example ship acorn which is not used outside of the REPL. We also have websockets implementation for the inspector but never expose it either.

We don't already include nodemon, so these examples aren't quite the same. Besides, as far as I understand it, in the case of acorn and websockets both were more or less required. nodemon is not required to fix an issue or compatibility problem that currently exists in node core itself.

Fighting to avoid adding any features that won't be used in all cases is tantamount to ripping out most of core so node is not usable on its own and all features must be bespoke and from the ecosystem.

That's a bit extreme. We've already had this small core discussion many times in the past, so I'm not going to repeat it here. However, I don't think there is anything wrong with wanting a small(er) node core (not just in literal size but also in scope) and not wanting to throw in the kitchen sink.

mscdex avatar Oct 13 '21 15:10 mscdex

I tend to agree we need something to solve this problem. I don't think we should be embedding nodemon - that's a solved problem at this point, I don't think we would stop people from using nodemon. I would like something better than nodemon.

However the hot reloading within devtools and import.meta.hot are important and currently impossible for us to solve for ESM.

mcollina avatar Oct 13 '21 15:10 mcollina

impossible for us to solve for ESM

due to some limitations in the current node ESM loader implementation?

capaj avatar Oct 13 '21 16:10 capaj

due to some limitations in the current node ESM loader implementation?

As far as I know it's currently impossible to implement hot module reloading in ESM.

mcollina avatar Oct 13 '21 16:10 mcollina

However the hot reloading within devtools and import.meta.hot are important and currently impossible for us to solve for ESM.

@mcollina is your assertion that a --watch feature might solve this, in line with what @bmeck mentioned, or that --watch also won't solve this? Just want to be sure.

bnb avatar Oct 13 '21 16:10 bnb

due to some limitations in the current node ESM loader implementation?

No. This is due to how caching is constrained by the specification, once a URL is used it is permanently cached and cannot be unloaded.

Besides, as far as I understand it, in the case of acorn and websockets both were more or less required.

In both cases there is no requirement that node support users wishing to use the features these provide.

bmeck avatar Oct 13 '21 16:10 bmeck

@bnb a watch mechanism could automatically propagate an import.meta.hot signal for reloading yes.

bmeck avatar Oct 13 '21 16:10 bmeck

@bnb what @bmeck and I are saying is that it's not possible to do hot reloading right now with ESM.

mcollina avatar Oct 13 '21 17:10 mcollina

Well, you can reload... but it leaks memory and under the hood rewrites URLs (would still be true with any sort of --watch), we have a PR with a test to show that mocking/aliasing a URL works but the only way to not leak is to have some sort of manager like reloading and an opt-out mechanism like import.meta.hot to avoid unnecessary reloading.

bmeck avatar Oct 13 '21 17:10 bmeck

ping on this.

bnb avatar Feb 01 '22 16:02 bnb

@bnb what's the next step?

targos avatar Feb 01 '22 17:02 targos

my understanding of next steps:

it (generally) seems like there's agreement that we'd like to see something along these lines. defining the scope of what that is and isn't, including the feedback from @mcollina and @bmeck, would be helpful in guiding an actual eventual implementation.

bnb avatar Feb 01 '22 18:02 bnb

I think agreement on scope would be a good start. Having a process manager vs having a workflow to reload in the same process is one scope argument. Having solutions for C++ add-ons, WASM, and workers/cluster for scoping in both cases likely differ greatly. After scoping, determining mechanisms to instrument reloading is likely a secondary concern.

On Tue, Feb 1, 2022, 12:26 PM Tierney Cyren @.***> wrote:

my understanding of next steps:

it (generally) seems like there's agreement that we'd like to see something along these lines. defining the scope of what that is and isn't, including the feedback from @mcollina https://github.com/mcollina and @bmeck https://github.com/bmeck, would be helpful in guiding an actual eventual implementation.

— Reply to this email directly, view it on GitHub https://github.com/nodejs/node/issues/40429#issuecomment-1027154308, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABZJI77KXIQQ6BG5ERHIH3UZAQUDANCNFSM5F3DLGJQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

bmeck avatar Feb 01 '22 19:02 bmeck

@bnb a preliminary step is to embed https://www.npmjs.com/package/fsevents in core to replace the buggy fs watch logic we have for macs (might be a new API). There might be another issue around this somewhere.

I'm happy to join a call to scope this.

mcollina avatar Feb 01 '22 19:02 mcollina

@mcollina the documentation says that we use fsevents on mac: https://nodejs.org/dist/latest-v17.x/docs/api/fs.html#availability

What is buggy about what we do? Is there an issue about it?

targos avatar Feb 01 '22 20:02 targos

I don't know, maybe @pipobscure can help clarify.

mcollina avatar Feb 02 '22 19:02 mcollina

The latest libuv does use fsevents under the hood these days. This was an effort undertaken by libuv a while back after chatting about moving fsevents into node core at a conference (again quite a while back).

This wasn’t always the case. Originally libuv was using posix stuff for monitoring files and would quickly run into ulimit for file handle if monitoring deep directories (one handle per directory/file).

So for tools like nodemon I’d recommend moving to the builtin fs watch capabilities as they should be sufficient.

Fsevents offers more granularity and more history, so it’s still useful though for a much smaller number of cases. It’s the underlying technology for time machine, so you can actually go back in time (before process start) and recall events as well. Which is great, but not all that useful for something like nodemon.

So from my perspective there’s nothing that would prevent just using the builtin stuff here. That being said, I have not tried out the fsevents implementation if libuv, so it may be worth doing an experiment. But if something doesn’t work quite right I’d think the place to fix it is in libuv.

pipobscure avatar Feb 03 '22 11:02 pipobscure

Thanks @pipobscure for the explanation!

mcollina avatar Feb 03 '22 11:02 mcollina

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

github-actions[bot] avatar Aug 03 '22 01:08 github-actions[bot]

@mcollina does @pipobscure's context help with the necessary step of using fsevents in core?

bnb avatar Aug 03 '22 05:08 bnb

What I understand from https://github.com/nodejs/node/issues/40429#issuecomment-1028912410 is that we should be able to just use fs(Promises).watch.

targos avatar Aug 03 '22 06:08 targos

I think this would be an amazing feature to add to core.

Are there any volunteers? We should remember to ping nodemon maintainers before hand.

mcollina avatar Aug 03 '22 18:08 mcollina

Are there any volunteers?

I'd happily give it a try

We should remember to ping nodemon maintainers before hand.

Who needs to be pinged? What message should be communicated?

MoLow avatar Aug 03 '22 18:08 MoLow

@MoLow probably the people who are actively committing to nodemon + anyone else listed as a maintainer on the npm page. I'd say that we're considering adding a nodemon-like feature to core and that we wanted to let them know, plus extend an offer to work with them and see how we can support them and learn from lessons they may have learned over the years.

bnb avatar Aug 03 '22 20:08 bnb

(also @mcollina and others might have additional thoughts on what we should communicate!)

bnb avatar Aug 03 '22 20:08 bnb

what @bnb said!

mcollina avatar Aug 03 '22 21:08 mcollina

@remy I tried contacting you via email without any success, trying here: we are considering adding watch (/nodmon like) capabilities into node core and would love to hear some feedback from you regarding this.

MoLow avatar Aug 09 '22 16:08 MoLow

@MoLow hey - sorry, read your email at totally the wrong time, and thusly forgot instantly!

Happy to offer advise, insights and contribute if it's useful. Nodemon has a surprisingly broad use, though rooted in node-land because of my own background.

An important part of the puzzle in nodemon is the Chokidar project, and I'd send up a red flag whenever "just" is used, it's nearly always where the pain points are:

we should be able to just use fs(Promises).watch.

Happy to give some insights into my design decisions in the software (the code itself… probably would look different if I wrote it today, but the user experience would be the same). The biggest driving principle is that it should be simple - i.e. no reading or prior knowledge required - to use.

This is also why I went for something that does a complete reload, that way all memory is trashed and the user's code starts from scratch (early on there was another program that you could stitch into your code and it would reload modules but my issue was the overhead of using and then disposing of the tool).

Some of the biggest and constant issues that have come up over the years is the sub-process being stopped or reloaded and it leaving ports open (and this happens in many different ways), so there's been work to add delays, checks, and all kinds.

I'd imagine if reload was built into node, it probably shouldn't try to do All The Things, but rely on as set of defined agreements. Though, and I'm guessing a little here, if node is internally able to track what files have been loaded into memory, then it makes the watch list very simple to manage. Of course then there's the question of "does it reload on data or config change", and then the user who says "I don't want it to reload on data change…", etc.

I've been pretty much the sole developer on nodemon for the last… 12 years(!), but as I mentioned before, Chokidar is a core component and it wouldn't be possible without it (the early crude version did use .watch and it was quickly apparent that it fell short).

Also the FAQ might also be a good place to look to see what kind of problems people have run up against in the past (I've used it as "this issue keeps coming up in github, so let's doc it"): https://github.com/remy/nodemon/blob/main/faq.md

I'd imagine you'd have more questions, feel free to shoot here (or drop me a(nother) email and I'll actually reply!)

remy avatar Aug 09 '22 16:08 remy

@remy chokidar is basically a wrapper around fs.watch and fsevents. So in terms of that modern libuv provides a fs.watch that eliminates the shortcomings of earlier versions and obviates chokidar.

pipobscure avatar Aug 09 '22 21:08 pipobscure

I took a look at some existing solutions, including nodemon, node-dev and deno --watch, my conclusion is avoiding child processes will provide a nicer/smoother DX, I think the best way to do that would be in the c++ side of node (so it will work even when the event loop is bussy, and probably one thread can listen to fsevents and trigger re-creating & running the main thread isolate), I don't have lots of c++ experience so I wonder who can work on this together with me?

MoLow avatar Aug 18 '22 19:08 MoLow

@addaleax @jasnell I was told you might know who can help

MoLow avatar Aug 18 '22 19:08 MoLow

A common ask, which I think needs a sub process (but I could be wrong), is when devs use nodemon with --inspect.

remy avatar Aug 18 '22 19:08 remy

my conclusion is avoiding child processes will provide a nicer/smoother DX

Could you elaborate on that point?

cjihrig avatar Aug 18 '22 19:08 cjihrig

Could you elaborate on that point?

  1. my assumption, (it might be wrong) is that using sub-processes has a bigger performance penalty than using a thread for handling file system events.
  2. using sub-processes can introduce some edge cases that need to be handled, for example, what happens when the sub-process is killed using the OS?
  3. some features depending on a single process/pid are now a little bit more confusing and hard to use (e.g SIGUSR1, spawn with bot --watch and ipc)

if we use sub-processes we might as well vendor nodemon

MoLow avatar Aug 18 '22 20:08 MoLow

We also have a watcher in tsx, which works similarly to node-dev as it only watches the dependency tree (but also supports native ESM).

It also spawns a child process which has been working great. No noticeable performance drawbacks and I think the implementation is minimal & maintainable.


Another file watcher alternative to consider is @parcel/watcher, which leverages FSEvents/Watchman/inotify/etc.

Apparently it was built to address the unreliability of chokidar and is now even used by VSCode.

privatenumber avatar Aug 18 '22 20:08 privatenumber

@MoLow nice. Now wondering if nodemon just got it's death sentence 😆 😢

remy avatar Sep 05 '22 09:09 remy

@remy I don't think so. First, this has a long way to go before reaching feature parity (i.e. https://github.com/remy/nodemon/blob/HEAD/doc/requireable.md is missing). Second, this is hardly stable, and it will require a few years before it is rolled out to all Node.js versions. Third, everybody using your software would hardly switch.

mcollina avatar Sep 05 '22 10:09 mcollina

:D yeah, the old dogs and new tricks thing will probably keep folks using nodemon for many a year.

Good addition to node either way 👍

remy avatar Sep 05 '22 10:09 remy