Webpack: Resolve assets watch not recompiling

[Samk13]

❤️ 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:

• I'm aware of the code of conduct.
• I've created logical separate commits and followed the commit message format.
• I've added relevant test cases.
• I've added relevant documentation.
• I've marked translation strings (for relevant code).
• I've followed the CSS/JS and React guidelines (for relevant code).
• I've followed the web accessibility guidelines (for relevant code).
• I've followed the user interface guidelines (for relevant code).
• I've identified the copyright holder(s) and updated copyright headers for touched files (>15 lines contributions).
• I've NOT included third-party code (copy/pasted source code or new dependencies).

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:

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

[rekt-hard]

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

[Samk13]

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]

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

[jrcastro2]

@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]

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.

[Samk13]

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]

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]

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]

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]

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]

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]

@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]

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]

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]

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!