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

[1.4.2] Changes in npm linked modules no longer cause a rebuild #7978

Closed
damonmaria opened this Issue Oct 27, 2016 · 17 comments

Comments

Projects
None yet
5 participants
@damonmaria

damonmaria commented Oct 27, 2016

My guess is in improving build speed this functionality may have been dropped intentionally, but maybe it's an oversight.

In 1.4.1 if I updated any of the several npm packages I am working on that are npm linked to the node_modules of the Meteor project then Meteor would rebuild. This no longer happens in 1.4.2.

Is this the way it's supposed to be?

MacOS 10.12.1

@benjamn

This comment has been minimized.

Member

benjamn commented Oct 27, 2016

Yes, starting watchers for files in node_modules was prohibitively expensive, so now (in Meteor 1.4.2) we watch for changes only in the contents of node_modules directories. I'd be happy to consider further refinements, but it can't involve watching everything in node_modules.

@benjamn benjamn added this to the Release 1.4.2.1 milestone Oct 27, 2016

@benjamn benjamn self-assigned this Oct 27, 2016

@benjamn benjamn referenced this issue Oct 27, 2016

Merged

Release 1.4.2 #7668

@damonmaria

This comment has been minimized.

damonmaria commented Oct 28, 2016

Totally understand. And I'm willing to lose linked modules causing a rebuild for faster build speed.

But I wonder if a child of node_modules is a symlink (should be a good determinant of npm lnked) then it could be 'deep watched'? We've moved a lot of our common Meteor code out to npm modules and a lot of our editing goes on in those rather than the project we're working on.

I just tried to see if I could workaround this manually and trigger a rebuild by touching the symlink but that didn't work. But then I tried it on an ordinary directory in node_modules (react) and that didn't trigger a rebuild either. So maybe fsnotify or whatever it is doesn't see that as a change.

Another possibility is to expose in Meteor shell a way to invalidate a path so we can trigger a rebuild if we want.

Also, just to complicate things, all our packages are npm scoped packages and so the actual package dir is not directly under node_modules. Just mentioning that in case it affects anything.

@jchristman

This comment has been minimized.

jchristman commented Oct 28, 2016

I second the idea of 'deep watching' symlinks in node_modules. The only way that anything should be linked is if someone is intending on editing that module without having to reinstall or update every time they make a change. I think that is likely the simplest solution.

@hexsprite

This comment has been minimized.

Contributor

hexsprite commented Oct 28, 2016

the only problem with deep watching symlinks is that they often have their own node_modules inside them... so maybe that could be excluded.

@benjamn

This comment has been minimized.

Member

benjamn commented Oct 28, 2016

Yep, I think this would only apply to symlinks in the top-level node_modules directory in the app, right?

@jchristman

This comment has been minimized.

jchristman commented Oct 28, 2016

Tldr; watch all symlinks and their subsequent symlinks in their node_modules, but don't watch the normal node_modules of the symlinked things.

Well - there is theoretically the use case where A is symlinked to B, which is symlinked to project C, yes? In my specific use case, I only have devDependencies in the node_modules of the thing that is symlinked, because when I get around to publishing it, the dependencies will be installed. But I do believe it is in fact possible to have a linked module depend on another linked module. So maybe watch all symlinks to an arbitrary depth, but don't watch anything else? I understand the performance hit here, but the ability to have this use case may prove useful to someone?

benjamn added a commit that referenced this issue Oct 28, 2016

@benjamn

This comment has been minimized.

Member

benjamn commented Oct 28, 2016

This should be fixed if you run meteor update --release 1.4.2.1-beta.0 in your app directory.

@jchristman

This comment has been minimized.

jchristman commented Oct 28, 2016

Awesome I'm loving the quick release cycle with the betas. You guys rock!

@damonmaria

This comment has been minimized.

damonmaria commented Oct 29, 2016

Just one thing. We use @Scoped packages. So the symlinks to our package are 2 levels deep.

e.g. node_modules/@mindhive/components where components is the symlink.

That's for the super quick update. Wow.

@damonmaria

This comment has been minimized.

damonmaria commented Oct 31, 2016

FYI I'm not getting any rebuilds from updating my npm linked packages using beta 0. Not sure if it's because they're scoped packages like mentioned above, or another reason.

benjamn added a commit that referenced this issue Oct 31, 2016

@benjamn

This comment has been minimized.

Member

benjamn commented Nov 3, 2016

When you get a moment, please confirm whether this is fixed (especially for @Scoped packages) by meteor update --release 1.4.2.1-beta.1. Closing now because it seems to be fixed in my tests, but of course feel free to reopen if you discover otherwise!

@benjamn benjamn closed this Nov 3, 2016

@damonmaria

This comment has been minimized.

damonmaria commented Nov 3, 2016

I can confirm (for me) that Meteor 1.4.2.1-beta-1 rebuilds with updates to @Scoped linked packages. And rebuilds pretty damn quick I might add.

Thanks for the quick fix @benjamn. Mighty impressed.

@benjamn

This comment has been minimized.

Member

benjamn commented Nov 3, 2016

Sweet!

@mjgallag

This comment has been minimized.

mjgallag commented Nov 3, 2016

@benjamn With 1.4.2.1-rc.0, if I delete index.js it triggers a rebuild, but when I recreate index.js it doesn't trigger a rebuild.

@mjgallag

This comment has been minimized.

mjgallag commented Nov 10, 2016

@benjamn with 1.4.2.1 an update to the main js file triggers a rebuild. Updates to js files imported by the main js file don't trigger rebuilds. Let me know if I should open a new issue. Thanks for your help.

@mjgallag

This comment has been minimized.

mjgallag commented Nov 15, 2016

@benjamn with 1.4.2.2-rc.1 I no longer have to touch my main js file to trigger a rebuild, thank you for the quick fix!

@benjamn

This comment has been minimized.

Member

benjamn commented Nov 15, 2016

Good to know that e4c7b08 did the trick!

@benjamn benjamn referenced this issue Nov 15, 2016

Merged

Release 1.4.2.2 (and 1.4.2.3) #8044

5 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment