Feature/node 18 decrease development reload time #54
Conversation
- watch all file types (config files might contain other types (non js / ts, etc)) - Use rsync to sync /app to /usr/src/app (excluding node_modules), instead of moving node_modules back and forth
|
Hi! This PR is a bit empty in verbal explanation. Could you add some performance benchmark improvements or indicate on which systems you noticed an improvement or which sort of applications seem to require an improvement in this regard? Perhaps also add a general rationale? Thanks! |
|
This is what bart sent me in the chat:
|
|
@gauquiebart could you rebase this on master? |
|
Ok, that makes sense. It's nice to receive a PR with a description in it though, especially knowing it's from people that can make a clean write-up. At this point it's a bit rude. Some comments based on what I read in code but without executing it or knowing what has been thought about and what not: Watching all files is ill-advised (and probably is a totally different feature than using rsync for performance reasons). Depending on the editor and toolchain used, it's common that temporary files are added to the folder containing the sources (think debug logs of a language server, temporary files, or configuration folders that serve more of a marketing purpose than anything else). Watching those will make the live reload go crazy. The removal of newlines probably has nothing to do with the PR. I think the provided rsync command drops support for starting off from the locally available node modules folder. I believe that is currently copied over when the service starts for the first time which I don't think is still the case with the provided rsync command. I did not check it specifically though. Perhaps you did. Perhaps that's a good addition to the description. I also wonder if this has an impact on Linux systems, which I think @nvdk studied anecdotally. |
|
|
||
| ## Copy app folder and config folder (including node_modules so host node_modules win) | ||
| cp -rf /app ./ | ||
| rsync --delete -a --exclude=node_modules /app/ app/ |
There was a problem hiding this comment.
Using rsync with the --exclude=node_modules changes behavior compared to before. Previously the node_modules from the mounted volume in /app were taken into account and combined with the node_modules from a previous build cycle. Now they're not anymore. It will always only use the node_modules from a previous build cycle.
|
@gauquiebart Can you have a look at #58 to see whether this decreases reload time on your machine? It's inspired on this PR. |
|
Had some problems with this version feature-node-18 on my Mac m2, however this was completely fixed with the latest image, and makes it about 15 times faster for me. If anybody wants some more feedback or if I need to run some version to test, feel free to ask. |
|
I think this PR can be closed? given the feedback from @bfidlers ? |
|
Closed in favor of #58 |
No description provided.