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

Files inside nested node_modules are cached until restart #10664

Closed
arggh opened this issue Aug 4, 2019 · 7 comments
Closed

Files inside nested node_modules are cached until restart #10664

arggh opened this issue Aug 4, 2019 · 7 comments

Comments

@arggh
Copy link
Contributor

arggh commented Aug 4, 2019

Rich Harris shared a "JS lifehack": put common utilities in /src/node_modules or /client/node_modulesetc. and import them easily without the ../../../../../utils/module.js ritual.

It works in Meteor, but changing the code in those files located in this extra node_modules directory won't trigger a refresh in Meteor. The code that was loaded on startup is cached forever until a full restart happens.

Symlinking to /imports doesn't help.

Meteor version used: 1.8.2-beta.16

@arggh
Copy link
Contributor Author

arggh commented Aug 4, 2019

This might be relevant to what's going on in #10522

@benjamn
Copy link
Contributor

benjamn commented Sep 10, 2019

There are two main reasons we don't watch (most) files in node_modules:

  1. There are just too many files in most node_modules directories (vastly more than the application itself, typically), so even stat-polling-based watching becomes prohibitively expensive, and fs-event-based watching quickly exhausts the available file descriptors.
  2. Compared to application code, we don't expect node_modules files to change very often except when doing an npm install, so picking up the changes on the next restart seemed (at the time this decision was made) to be a good compromise.

I'm not attempting to shoot down this idea, because I agree there are cases where you might be actively editing some files inside node_modules, but the solution cannot be to watch everything in node_modules. Whenever we reach this sort of dilemma, we need to empower the developer to choose which bits within node_modules they want to watch, which is a design problem that requires some creativity and taste.

In this particular case, I suppose we could watch node_modules directories other than / outside of the root node_modules directory, like /client/node_modules/.... That would be a relatively easy change to the shouldWatch function, I think.

As a historical note, we do watch node_modules/linked-package-name if that package directory is a symbolic link (see this line), since that's how npm link installs packages, and presumably npm link-ed packages are under active development (compared to normally-installed packages): #7978

@CaptainN
Copy link
Collaborator

I usually just import from root, and am perplexed by recent linter rules showing up in my configuration which prohibit that. A statement like import { thing } from '/imports/utils/thing' is infinitely more readable/understandable/useful than import { thing } from '../../../../../utils/thing.js'.

For watching - I noticed that even node_modules listed to in meteor -> nodeModules -> recompile are not watched. If that were changed, maybe that can be a way to add a watch exception?

@benjamn
Copy link
Contributor

benjamn commented Sep 10, 2019

For watching - I noticed that even node_modules listed to in meteor -> nodeModules -> recompile are not watched. If that were changed, maybe that can be a way to add a watch exception?

That's definitely doable! We could also introduce a meteor.nodeModules.watch list, which could simply be a list of package names, without the additional architecture stuff that comes with meteor.nodeModules.recompile. I'm inclined to say recompile should imply watch, but not the other way around.

This may just be me rationalizing a new configuration option, but it's nice when a parent section has at least two possible sub-sections. Right now meteor.nodeModules.recompile is the only reason you'd need a meteor.nodeModules section, so adding meteor.nodeModules.watch would help flesh it out a bit?

@arggh
Copy link
Contributor Author

arggh commented Sep 11, 2019

In this particular case, I suppose we could watch node_modules directories other than / outside of the root node_modules directory, like /client/node_modules/.... That would be a relatively easy change to the shouldWatch function, I think.

Not exactly my area of expertise to consider all the weird and exciting patterns developers use in the wild, but I'd assume that for most Meteor apps, watching node_modules other than the root node_modules would be a zero cost operation, since those folders just don't exist?

I would also be happy to just configure the paths to the watched modules.

We could also introduce a meteor.nodeModules.watch list, which could simply be a list of package names, without the additional architecture stuff that comes with meteor.nodeModules.recompile

In that case, what do you think about allowing globs or patterns to be defined for meteor.nodeModules.watch ?

"meteor": {
  "nodeModules": {
    "watch": [
       "client/node_modules/**/*"
    ]
  }
}

Would be nicer, than keeping an up-to-date list of folders (that is actually application code). At least I would, from time to time, with 100% certainty, end up wondering why changes are not picked up when I forget to add an entry to package.json when new modules are introduced.


→ From the two options, I'm in favor of just treating node_modules outside of the root folder as application code.

@CaptainN
Copy link
Collaborator

@benjamn If we are looking for rationalizations for the "nodeModules" json node, how about adding package aliases! It'd be great in some cases to replace react (and react-dom) with preact (and preact-compat) client side.

@benjamn benjamn added this to the Release 1.8.2 milestone Sep 18, 2019
@benjamn benjamn self-assigned this Sep 18, 2019
benjamn added a commit that referenced this issue Sep 18, 2019
This function determines whether the optimistic caching system should
subscribe to file change notifications for a given path.

Note that Meteor has other ways of watching for file changes (e.g. the
WatchSet abstraction), as well as other mechanisms for invalidating cached
results, so time and resources can often be saved by returning false here,
if we know that the path will be watched in other ways.

Improvements:

- Add METEOR_PROFILE instrumentation to the shouldWatch implementation.

- Stop exporting shouldWatch, since it is no longer used anywhere else in
  the codebase.

- Return false early to avoid watching files inside the .meteor directory,
  a significant shortcut.

- Allow watching files in application node_modules directories that are
  not located directly in the root application directory (closes #10664).
benjamn added a commit that referenced this issue Sep 18, 2019
This function determines whether the optimistic caching system should
subscribe to file change notifications for a given path.

Note that Meteor has other ways of watching for file changes (e.g. the
WatchSet abstraction), as well as other mechanisms for invalidating cached
results, so time and resources can often be saved by returning false here,
if we know that the path will be watched in other ways.

Improvements:

- Add METEOR_PROFILE instrumentation to the shouldWatch implementation.

- Stop exporting shouldWatch, since it is no longer used anywhere else in
  the codebase.

- Return false early to avoid watching files inside the .meteor directory,
  a significant shortcut.

- Allow watching files in application node_modules directories that are
  not located directly in the root application directory (closes #10664).
@benjamn
Copy link
Contributor

benjamn commented Sep 19, 2019

This should be fixed if you run meteor update --release 1.8.2-rc.0. I went with the strategy of watching node_modules directories that are not directly in the root application directory, so no additional configuration should be necessary.

Please feel free to reopen this issue if you continue to encounter similar problems, and thanks for all of your suggestions above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants