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

Server always rebuilds even when only client side javascript changed. #10591

Closed
Salketer opened this issue Jun 21, 2019 · 28 comments · Fixed by #11474
Closed

Server always rebuilds even when only client side javascript changed. #10591

Salketer opened this issue Jun 21, 2019 · 28 comments · Fixed by #11474
Labels
confirmed We want to fix or implement it in-development We are already working on it Project:Isobuild
Milestone

Comments

@Salketer
Copy link

I see a couple complains in the form of #10449 about meteor packages causing server rebuilds.

But without using packages, the behavior is the same.

meteor create --full serverBuildRepro

Changing something in /imports/ui/pages/home/home.js causes a server rebuild, while changing something in /imports/ui/pages/home/home.html causes only a client rebuild.

I have tried to move back to 1.8.0.1 and the result is the same.

meteor --version
Meteor 1.8.1

Digging a bit more, it comes to the fact that the ui directory is not inside a client directory. Putting it inside a "client" directory works. The thing is that /imports/ui/pages/home/home.js is only imported by the file /imports/startup/client/routes.js which is only imported by /imports/startup/client/index.js

I see no reason for that particular file to be imported/used server-side, but it still causes server rebuilds. I have also trying using console.log and throwing errors on the root level of the file and nothing impacts the server so I assume it really is not loaded by the server.

@Salketer Salketer changed the title Server always rebuilds javascript Server always rebuilds even when only client side javascript changed. Jun 24, 2019
@aogaili
Copy link

aogaili commented Jun 24, 2019

That was the original behaviour, before ES imports, thing under the client folder will only be imported at the client and will result in client only refresh.

Benjamn mentioned that he has plan to generalize this behaviour (not depending on the client folder) for both the main app and the packages, which I'm really looking forward to!

@Salketer
Copy link
Author

Salketer commented Jun 24, 2019 via email

@aogaili
Copy link

aogaili commented Jun 24, 2019

Are they eagerly loaded? I don't think so, I think it just impact the dev refresh behaviour, but maybe I'm wrong. Either way, since the client files can be inferred by tracing the import dependency tree starting from the client entry point, I think the refresh behaviour can be generalized beyond the client folder, for both the app and packages, which I think is what you looking for.

@sakulstra
Copy link
Contributor

Benjamn mentioned that he has plan to generalize this behaviour (not depending on the client folder) for both the main app and the packages, which I'm really looking forward to!

Would this also help with issues like #10106? We've moved to a multi package.json architecture to have jest, eslint and other dev only depencencies in another node_modules outside the meteor app... which is pretty annoying as we have to configure jest and webpack to resolve modules from multiple folders causing hard to debug bugs from time to time.

@Salketer
Copy link
Author

@sakulstra I am referring to a completely different thing here... Nothing to do with jest, server-side code or windows vs linux. This may incidentally produce less rebuilds so in the end speed up your devs tough.

@aliogaili I double checked, I did not use the right vocabulary. They are in fact not eagerly loaded. As I was myself pointing out that the console.logs weren't showing. So they are not used at all by the server code but the file watcher seems to trigger both reloads anyways since it is not inside a client parent folder.

@smeijer
Copy link

smeijer commented Nov 20, 2019

Having the same issue on 1.8.2, even while that should be fixed(/improved?) in #10686.

Is there a way to find out 'why' Meteor decides to trigger a rebuild of either side?

Something like; rebuild server because a detected change in "x"?

I've browsed the server bundle trough the (chrome) node-inspector, but the file I touch isn't listed there.

@smeijer
Copy link

smeijer commented Nov 22, 2019

Even with an empty server/main.js, the server still rebuilds. Not on every file though, but most of them do.

All my code lives in /src/common, except for the two main modules:

    "mainModule": {
      "client": "./src/client/main.js",
      "server": "./src/server/main.js"
    }

@benjamn
Copy link
Contributor

benjamn commented Nov 22, 2019

@smeijer Does your application use the autoupdate package (check meteor list --tree if unsure)? That's the package that implements client refreshes, so the server will always restart if you don't have it installed.

@Salketer Using the final/official release of Meteor 1.8.2, I can no longer reproduce the server refresh for edits to /imports/ui/pages/home/home.js in a --full application (which I agree should not cause a server restart). Perhaps the behavior improved since you first diagnosed this bug?

@smeijer
Copy link

smeijer commented Nov 23, 2019

@benjamn , it does:

meteor-base@1.4.0
├─┬ hot-code-push@1.0.4
│ ├─┬ autoupdate@1.6.0
│ │ ├── check@1.3.1 (top level)
│ │ ├── ...
│ └── reload@1.3.0 (top level)

@stale
Copy link

stale bot commented Dec 25, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale-bot label Dec 25, 2019
@smeijer
Copy link

smeijer commented Dec 27, 2019

adding activity

@stale stale bot removed the stale-bot label Dec 27, 2019
@aogaili
Copy link

aogaili commented Dec 27, 2019

I thought this was resolved in latest version? I've not tested it yet.

@sebakerckhof
Copy link
Contributor

@smeijer is there a reproduction you can share?

@smeijer
Copy link

smeijer commented Jan 8, 2020

It's a large application that I then should somehow try to slim down. As I said, it's not every file that triggers it.

Is there a way to log which dependency path triggered the rebuild? Something like

server restarted because of change in "./common/foo.js"
 imported by `./server/bar.js`
 imported by `./server/main.js`

?

@sebakerckhof
Copy link
Contributor

sebakerckhof commented Jan 8, 2020

I agree that would be useful. There is currently no easy way to do this. Note that there may be multiple paths.
It's doable though, but from a quick glance you would need to:

  1. let the watchset expose what triggered the change
  2. let the import scanner keep track of all parents for each file (currently it only keeps track of childs)
  3. work around circular imports etc.

It's something I could have used in the past already, so maybe I'll see if I can tackle this next week, but feel free to beat me to it.

@smeijer
Copy link

smeijer commented Jan 8, 2020

Turns out, the reproduction is quite simple. It looks to me that the server still always rebuilds when changes are detected that were not in a imports directory.

If I understand correctly, the imports directory shouldn't be necessary when making use of the meteor.mainModule setting, as mentioned here: meteor/meteor-feature-requests#135 (comment), yet changes outside the imports directory, still trigger a server restart.

meteor-rebuild-issue

I have a repo here: https://github.com/smeijer/meteor-rebuild-issue/

To recreate this repo from scratch:

meteor create meteor-rebuild-issue --release 1.8.3 --react
cd meteor-rebuild-issue
meteor

Now edit ./imports/ui/Hello.jsx, and notice that only the client bundle is being rebuilt.

Next, rename ./imports to ./common, and update the import statement in ./client/main.jsx and ./server/main.js to reflect the new path. Wait for the application to rebuild.

Now, go ahead and change something in ./common/ui/Hello.jsx.

You'll notice that the server is rebuilt as well, while changes inside the imports directory did not.

@smeijer
Copy link

smeijer commented Jan 8, 2020

After some more testing, my conclusion is that meteor.mainModule disables eager loading for files that are not inside the imports directory, but it doesn't have any effect on the watching of those files. So even though they are not being (eagerly) loaded, they still trigger restarts.

meteor-rebuild-issue

@sebakerckhof
Copy link
Contributor

@smeijer yes, that's because Meteor has to watch all those directories in case you have commonjs requires, which are not statically analyzable.

E.g. you can do something like:
require(someVariable);
So therefore Meteor has to watch all the directories (although it filters out 'client' from the server and vice-versa).

If you would only have es6 imports, we can just rely on Meteor's ImportScanner to add the necessary files, but unfortunately that's not an assumption we can make.

@smeijer
Copy link

smeijer commented Jan 16, 2020

@sebakerckhof, that makes sense, but it is really unfortunate.

Would it be possible to add a setting to the package.json to force Meteor to make that es6 import only assumption? That would reduce the number of rebuilds greatly.

A more advanced solution would perhaps be to see if the imported path has any require statements, but for me, just promising that I will only use es6 imports would be fine.

@sebakerckhof
Copy link
Contributor

Yeah, but Meteor tries to be a zero-config tool as much as possible, so I'm personally not in favor of adding an such an option.

It could be automated. Meteor already scans for imports/requires etc. in the ImportScanner, so it wouldn't add much cost, but this currently happens way after the watches are added.

Plus, There might be more reasons to watch the files I don't know about (but @benjamn does).

Anyway, I made a quick and dirty version which allows to skip watching with an envar when you only have static imports, you can try it out, by cloning this branch: https://github.com/sebakerckhof/meteor/tree/feat/watchset-feedback
And running:
ONLY_IMPORTS=1 path/to/meteor/meteor

@sebakerckhof
Copy link
Contributor

Actually turns out @zodern already has a PR solving this nicely: #10455

@smeijer
Copy link

smeijer commented Feb 21, 2020

It would be nice if #10455 could be continued or even getting merged. It would solve the trouble of long rebuild times due to a server refresh when only the client is being updated.

@stale
Copy link

stale bot commented Mar 24, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale-bot label Mar 24, 2020
@zodern zodern added the pinned label Mar 24, 2020
@stale stale bot removed the stale-bot label Mar 24, 2020
@stale
Copy link

stale bot commented Oct 31, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale-bot label Oct 31, 2020
@smeijer
Copy link

smeijer commented Nov 1, 2020

Ping

@filipenevola
Copy link
Collaborator

filipenevola commented Apr 9, 2021

Hi, we have a PR that is passing with changes to fix this issue. PR #11381

We should have a review next week so we can proceed.

@filipenevola filipenevola added the in-development We are already working on it label Apr 9, 2021
@StorytellerCZ
Copy link
Collaborator

Possible duplicate of #9960

@filipenevola
Copy link
Collaborator

This PR #11474 should finally fix this soon 😉

Keep watching!

@StorytellerCZ StorytellerCZ linked a pull request Jun 12, 2021 that will close this issue
@StorytellerCZ StorytellerCZ added this to the Release 2.3 milestone Jun 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed We want to fix or implement it in-development We are already working on it Project:Isobuild
Projects
None yet
9 participants