Webpack: Resolve `assets watch` not recompiling #166

Samk13 · 2023-12-23T20:04:12Z

❤️ Thank you for your contribution!

Description

This PR:

Closes #2557

Question

While debugging, I found that CleanWebpackPlugin plugin, becomes redundant after upgrading to webpack V5

invenio-assets/invenio_assets/assets/build/webpack.config.js

Lines 210 to 216 in f562914

    
           // Removes the dist folder before each run. 
        
           new CleanWebpackPlugin({ 
        
             dry: false, 
        
             verbose: false, 
        
             dangerouslyAllowCleanPatternsOutsideProject: true, 
        
             cleanStaleWebpackAssets: process.env.NODE_ENV === "production",   // keep stale assets in dev because of OS issues 
        
           }),

by adding clean: true to output, see cleaning-up-the-dist-folder
Did I overlooked another purpose for the CleanWebpackPlugin? 🤔

Checklist

Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:

Third-party code

If you've added third-party code (copy/pasted or new dependencies), please reach out to an architect.

Reminder

By using GitHub, you have already agreed to the GitHub’s Terms of Service including that:

You license your contribution under the same terms as the current repository’s license.
You agree that you have the right to license your contribution under the current repository’s license.

rekt-hard · 2024-01-23T13:49:11Z

Thank you! Works like a charm locally (though it will take some time (42s in my case))

Samk13 · 2024-01-26T13:48:52Z

Good to hear @rekt-hard, In my experience, the process was somewhat slow, taking up to ~15 - 20 seconds, but not excessively so.
I experimented with various caching strategies to enhance speed, yet these attempts didn't significantly boost performance.
I also explored the watching options detailed in this Webpack docs, but the improvements were minimal, so I excluded them from the current PR.
I am always open to further improving it if we can!

ntarocco · 2024-03-20T09:56:42Z

@jrcastro2 you also experienced this issue. How did you solve it?

jrcastro2 · 2024-03-20T13:46:24Z

@jrcastro2 you also experienced this issue. How did you solve it?

I remember experiencing this but I don't remember fixing it ...

jrcastro2

As you comment in the isuse ("While debugging, I found that CleanWebpackPlugin plugin, becomes redundant after upgrading to webpack V5"), have you tried to use the ouptut.clean?
I think that the CleanWebpackPlugin might not be working anymore.

jrcastro2 · 2024-03-20T13:50:22Z

invenio_assets/assets/build/webpack.config.js

@@ -252,6 +253,7 @@ var webpackConfig = {
  watchOptions: {
    followSymlinks: true,
  },
+  cache: process.env.NODE_ENV === "production",


Major: Based on the docs this is by default false in production, I guess we should keep the same behaviour, unless I am missing something.

Suggested change

cache: process.env.NODE_ENV === "production",

cache: false,

Here the docs.

Understood @jrcastro2 , but the caching seems to default to true after first compile despite what the docs say (it's why we have the issue in the first place). but If setting it explicitly to false is preferred, I'm on board with that change.

Samk13 · 2024-03-20T20:54:54Z

As you comment in the isuse ("While debugging, I found that CleanWebpackPlugin plugin, becomes redundant after upgrading to webpack V5"), have you tried to use the ouptut.clean? I think that the CleanWebpackPlugin might not be working anymore.

I inquired because, despite seeing no differences when I replaced the plugin with the output.clean option, I'm uncertain of the optimal way to confirm that nothing else is unintentionally affected. I checked config.build.assetsPath, and everything seemed fine.
We can make a separate commit to test this change if you'd like. Or, we can create a separate PR. WDYT @jrcastro2 ?

jrcastro2 · 2024-03-21T14:54:16Z

As you comment in the isuse ("While debugging, I found that CleanWebpackPlugin plugin, becomes redundant after upgrading to webpack V5"), have you tried to use the ouptut.clean? I think that the CleanWebpackPlugin might not be working anymore.

I inquired because, despite seeing no differences when I replaced the plugin with the output.clean option, I'm uncertain of

Did you test the output.clean without the cache option? My feeling is that the CleanWebpackPlugin might not be working well on the new versions. If setting output.clean to true fixes this, we avoid the performance issues that disabling the cache is causing.

Samk13 · 2024-03-21T15:22:22Z

Did you test the output.clean without the cache option? My feeling is that the CleanWebpackPlugin might not be working well on the new versions. If setting output.clean to true fixes this, we avoid the performance issues that disabling the cache is causing.

After conducting a test by eliminating the plugin and setting output.clean: true while also removing cache: false,
it's clear that cache: false is essential for the assets watch functionality to work. I also explicitly attempted to add cache.memory:

cache: {
  type: 'memory',
},

However, this did not resolve the recompiling issue either.
Do you think we should add to this PR the removal of the clean plugin in that case, as it seems redundant to me.

jrcastro2 · 2024-03-22T17:06:45Z

Did you test the output.clean without the cache option? My feeling is that the CleanWebpackPlugin might not be working well on the new versions. If setting output.clean to true fixes this, we avoid the performance issues that disabling the cache is causing.

After conducting a test by eliminating the plugin and setting output.clean: true while also removing cache: false, it's clear that cache: false is essential for the assets watch functionality to work. I also explicitly attempted to add cache.memory:
cache: {
  type: 'memory',
},
However, this did not resolve the recompiling issue either. Do you think we should add to this PR the removal of the clean plugin in that case, as it seems redundant to me.

Hmm, can you explain what issues do you still have after using the ouput.clean set to true?
I might miss some details but in my case testing with the following changes the updates of the less files are being picked as well as the changes of a locally installed package (I used invenio-theme and invenio-react-forms to test this).

Samk13 · 2024-03-22T20:02:41Z

Hmm, can you explain what issues do you still have after using the ouput.clean set to true?

A video might explain it better than words, so I'll share a screen recording with you, notice that only the first change is been picked up @jrcastro2
Am I missing something here?

Samk13 · 2024-03-23T01:07:41Z

@jrcastro2, in a separate commit after looking once again into it, I've removed several potentially outdated configs related to years-old issues (see gh links e.g. comparisons: false, or ascii_only: true from 2017 ) or webpack-livereload-plugin that last activity was more than 2 years ago. I know digging into these configs may not be pleasant while they don't affect caching directly. However, while we're on it, I need you to review and advise if we should eliminate more legacy options or reinstate any so I don't unintentionally impact the application due to unknown historical context or dependencies.

jrcastro2 · 2024-03-25T14:07:46Z

Hmm, can you explain what issues do you still have after using the ouput.clean set to true?

A video might explain it better than words, so I'll share a screen recording with you, notice that only the first change is been picked up @jrcastro2 Am I missing something here?

Ah okay, thanks for sharing this, looks like it's only affecting the updates on the instance folder, I think this is a "known issue" - if I am not mistaken. Thanks a lot for all the updates! I will have a look as soon as I am back (I am on holidays until April 3th).

And sorry for being so picky with the cache thing but I would like to avoid it if possible, to not slow it down, however I haven't found yet an alternative to it ... I will have a look as soon as I am back.

Samk13 · 2024-03-25T14:38:08Z

And sorry for being so picky with the cache thing but I would like to avoid it if possible, to not slow it down

I totally agree @jrcastro2 I, too, would favor an alternative solution than disabling the cache that allows for quicker recompiling. However, this is currently the only solution I've managed to make it work for development so far.
We can continue on this one when you are back!
Happy Easter! 🐣

jrcastro2 · 2024-04-04T08:39:25Z

After spending a bit of time exploring the options around the cache and trying to understand the issue better I couldn't find a better solution for the moment. It looks like the changes of js files are correclty picked up and only the changes on the less files (*.overrides and *.variables) of the instance folder are not being correctly picked up. To my understanding, the changes are detected and the compilation is trigerred but for some reason the bundle is not recreated (looks like is using the cached one).

Disabling the cache fixes the issue but it would slow down the development in general (there are some changes on js files that thake 1 second, while if we disable the cache any change will take +15 seconds, depending on how powerfull your local machine is).

Ideally, I would like understand how the cache is working in webpack and figure out why the bundle looks like is being incorrectly cached, but due to time constraints I cannot do this right now ... However, this is something we will have to address in the near future so it will happen, just on hold for now.

Regarding the other changes they look okay to me, I just would like to test the changes on the terserOptions in a production like environment just to be extra sure we don't miss anything.

Please, keep this PR open and we will resume the work from here as soon as possible, thank you for your contribution, it's very much appreciated!

Samk13 force-pushed the fix-assets-watch branch from 97184ba to d51eea7 Compare December 23, 2023 20:06

Samk13 changed the title ~~webpack: Resolve asset watching not compiling~~ webpack: Resolve assets watch not compiling Dec 23, 2023

Samk13 changed the title ~~webpack: Resolve assets watch not compiling~~ webpack: Resolve assets watch not recompiling Dec 23, 2023

Samk13 changed the title ~~webpack: Resolve assets watch not recompiling~~ Webpack: Resolve assets watch not recompiling Dec 24, 2023

Samk13 force-pushed the fix-assets-watch branch from d51eea7 to badad5a Compare January 3, 2024 22:02

jrcastro2 reviewed Mar 20, 2024

View reviewed changes

Samk13 force-pushed the fix-assets-watch branch from badad5a to c873d95 Compare March 20, 2024 14:15

webpack: Resolve asset watching not compiling

44113ab

Samk13 force-pushed the fix-assets-watch branch from c873d95 to 44113ab Compare March 20, 2024 14:16

ntarocco assigned jrcastro2 Mar 22, 2024

Samk13 force-pushed the fix-assets-watch branch from 23a3b39 to 59a85dc Compare March 23, 2024 00:56

webpack: Remove outdated config

749ae46

Samk13 force-pushed the fix-assets-watch branch from 59a85dc to 749ae46 Compare March 26, 2024 11:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Webpack: Resolve `assets watch` not recompiling #166

Webpack: Resolve `assets watch` not recompiling #166

Samk13 commented Dec 23, 2023 •

edited

rekt-hard commented Jan 23, 2024 •

edited

Samk13 commented Jan 26, 2024

ntarocco commented Mar 20, 2024 •

edited

jrcastro2 commented Mar 20, 2024

jrcastro2 left a comment

jrcastro2 Mar 20, 2024

Samk13 Mar 20, 2024 •

edited

Samk13 commented Mar 20, 2024

jrcastro2 commented Mar 21, 2024

Samk13 commented Mar 21, 2024 •

edited

jrcastro2 commented Mar 22, 2024

Samk13 commented Mar 22, 2024 •

edited

Samk13 commented Mar 23, 2024 •

edited

jrcastro2 commented Mar 25, 2024

Samk13 commented Mar 25, 2024 •

edited

jrcastro2 commented Apr 4, 2024

	// Removes the dist folder before each run.
	new CleanWebpackPlugin({
	dry: false,
	verbose: false,
	dangerouslyAllowCleanPatternsOutsideProject: true,
	cleanStaleWebpackAssets: process.env.NODE_ENV === "production", // keep stale assets in dev because of OS issues
	}),

Webpack: Resolve assets watch not recompiling #166

Are you sure you want to change the base?

Webpack: Resolve assets watch not recompiling #166

Conversation

Samk13 commented Dec 23, 2023 • edited

Description

Question

Checklist

rekt-hard commented Jan 23, 2024 • edited

Samk13 commented Jan 26, 2024

ntarocco commented Mar 20, 2024 • edited

jrcastro2 commented Mar 20, 2024

jrcastro2 left a comment

Choose a reason for hiding this comment

jrcastro2 Mar 20, 2024

Choose a reason for hiding this comment

Samk13 Mar 20, 2024 • edited

Choose a reason for hiding this comment

Samk13 commented Mar 20, 2024

jrcastro2 commented Mar 21, 2024

Samk13 commented Mar 21, 2024 • edited

jrcastro2 commented Mar 22, 2024

Samk13 commented Mar 22, 2024 • edited

Samk13 commented Mar 23, 2024 • edited

jrcastro2 commented Mar 25, 2024

Samk13 commented Mar 25, 2024 • edited

jrcastro2 commented Apr 4, 2024

Webpack: Resolve `assets watch` not recompiling #166

Webpack: Resolve `assets watch` not recompiling #166

Samk13 commented Dec 23, 2023 •

edited

rekt-hard commented Jan 23, 2024 •

edited

ntarocco commented Mar 20, 2024 •

edited

Samk13 Mar 20, 2024 •

edited

Samk13 commented Mar 21, 2024 •

edited

Samk13 commented Mar 22, 2024 •

edited

Samk13 commented Mar 23, 2024 •

edited

Samk13 commented Mar 25, 2024 •

edited