Do not share compilation state between plugin instances #52

satazor · 2018-08-29T20:10:00Z

The changes on #47 were causing problems when using multiple plugin instances. It's pretty common to have at least two plugin instances in the same node process in isomorphic projects.

The reason was that SvgSprite's were storing state between compilations, such as the dirty and updated flags, causing inconsistencies when different compilations were running concurrently.
To solve this, every plugin instance has its own compilation state, thanks to the SvgStorePluginSpriter.
Note that SvgSprite instances are still shared whenever possible.

Additional changes:

Allow partial options to be passed to the plugin
Moved the options parameter from SvgSprite's generate method to the constructor because it is a property that changes the content; this way the dirty and updated flag are always correct
Update example app scripts and add a start:prod script
Fix replacement of interpolated paths for modules of type CssModule
Fix errors being swalled in the recently added MissingDimensionsException

Fixes #53

satazor · 2018-08-29T20:20:44Z

@bensampaio I've updated some projects to the new version and I've detected an issue on one of them when doing a production build. This PR resolves that while keeping the performance improvements and the HMR on recompilations.

This was causing problems as SvgSprite's now have stateful properties, such as the `dirty`. To solve this, every plugin instance has it's own state, including their own SvgSprite instances. I've decided to make a refactor by moving the compilation related stuff to the SvgStorePluginSpriter. This new class is responsible for applying a single sprite to a compilation. The reason is that SvgStorePlugin was getting too big and confusing. Additional changes: - Changed resourcePath to path to avoid confusion with loader's resourcePath (relative vs absolute) - Moved the options parameter from SvgSprite's generate method to the constructor because it's a property that changes the content; this way the `dirty` flag is correct - Update example app scripts and add a start:prod script - Fix replacement of interpolated paths for modules of type CssModule - Fix errors being swalled in the recently added MissingDimensionsException Fixes bensampaio#52

satazor · 2018-08-30T16:23:49Z

@bensampaio I've updated the PR and it's now working correctly.

I've made an auxiliary refactor and some unrelated changes. Let me know if you like them or if you want them reverted. I've also updated the PR title and description accordingly.

This was causing problems as SvgSprite's now have stateful properties, such as the `dirty`. To solve this, every plugin instance has it's own state, including their own SvgSprite instances. I've decided to make a refactor by moving the compilation related stuff to the SvgStorePluginSpriter. This new class is responsible for applying a single sprite to a compilation. The reason is that SvgStorePlugin was getting too big and confusing. Additional changes: - Changed resourcePath to path to avoid confusion with loader's resourcePath (relative vs absolute) - Moved the options parameter from SvgSprite's generate method to the constructor because it's a property that changes the content; this way the `dirty` flag is correct - Update example app scripts and add a start:prod script - Fix replacement of interpolated paths for modules of type CssModule - Fix errors being swalled in the recently added MissingDimensionsException Fixes bensampaio#52

This was causing problems as SvgSprite's now have stateful properties, such as the `dirty`. To solve this, every plugin instance has it's own state, including their own SvgSprite instances. I've decided to make a refactor by moving the compilation related stuff to the SvgStorePluginSpriter. This new class is responsible for applying a single sprite to a compilation. The reason is that SvgStorePlugin was getting too big and confusing. Additional changes: - Moved the options parameter from SvgSprite's generate method to the constructor because it's a property that changes the content; this way the `dirty` flag is correct - Update example app scripts and add a start:prod script - Fix replacement of interpolated paths for modules of type CssModule - Fix errors being swalled in the recently added MissingDimensionsException Fixes bensampaio#52

bensampaio

@satazor I am a bit surprised with these changes... I am not sure I get all the changes you made so I left some comments. I am missing some documentation in the code explaining why certain changes are necessary.

I didn't have the time to properly evaluate the changes in SvgStorePlugin this time since they are quite extensive. Besides that, I noticed that you tried to use a deprecated variable so I would like first to know if there is a better alternative.

I also have doubts some of these changes are really scalable. Storing the content of icons, sprites and old sprites in memory doesn't really sound a very good idea... Or is this how other plugins implement HMR?

bensampaio · 2018-09-03T13:29:05Z

examples/react/package.json

+    "start:dev:no-hash": "EXAMPLE_NO_HASH=1 yarn start:dev",
+    "start:dev:hot-no-hash": "EXAMPLE_NO_HASH=1 yarn start:dev:hot",
+    "start:prod": "serve -l 3000 public/",
+    "build:prod": "NODE_ENV=production webpack -p"


@satazor nice addition! Can you change prod to prd though? That's what I use in all my project and I want to keep this consistent.

Oh I thought it was a mistake, will change.

bensampaio · 2018-09-03T13:31:55Z

index.js

@@ -34,7 +34,7 @@ const DEFAULT_QUERY_VALUES = {
 * @param {Buffer} content - the content of the SVG file.
 */
 function loader(content) {
-    const { addDependency, resourcePath } = this;
+    const { addDependency, resourcePath, _compiler } = this;


This is deprecated (as referred in https://webpack.js.org/api/loaders/#deprecated-context-properties) so we need a different approach.

Indeed, let me think a bit about it.

bensampaio · 2018-09-03T13:39:03Z

lib/SvgSprite.js

+     * @param {number} options.iconHeight - icon height in the sprite.
+     * @return {string}
+     */
+    static getId(resourcePath, options) {


What is the reason for this function? Is this related to the problem you are trying to fix?

The SvgStorePlugin has a store object that contains the SvgSprite instances. Previously, the keys of the store were the sprites' resourcePath. Though, this was causing problems when two SvgStorePlugin instances with different sprite options (positioning, spacing) were instantiated. More specifically, the cached content of each sprite would be wrong even if generate was called with different sprite options.

That's the reason why I moved the options from generate to the constructor in the SvgSprite. Moreover, the keys of the store are now a unique identifier of each SvgSprite instance, which takes into consideration the options by computing a hash of it.

Note that this method currently lives here but it can be moved to the SvgStorePluginSpriter instead.

Actually, the explanation is in a comment on the SvgStorePluginSpriter. Want me to move the explanation to here or move the method to SvgStorePluginSpriter?

This function was removed because it's no longer necessary.

bensampaio · 2018-09-03T13:41:21Z

lib/SvgSprite.js

@@ -75,40 +103,26 @@ class SvgSprite {
     * @returns {SvgIcon}
     */
    addIcon(resourcePath, name, content) {
-        const icons = this.icons;
-        const icon = icons[resourcePath] = new SvgIcon(this, name, content);
+        const icon = this.icons[resourcePath] = new SvgIcon(this, name, content);


Maybe it would be more readable to do this like:

const icon = new SvgIcon(this, name, content); this.icons[resourcePath] = icon; this.dirty = true; return icon;

Will change.

lib/SvgSprite.js

bensampaio · 2018-09-03T13:56:29Z

lib/SvgStorePluginSpriter.js

+        /** @member {RegExp} */
+        this.originalResourcePathRegExp = new RegExp(escapeRegExp(svgSprite.originalResourcePath), 'gm');
+        /** @member {?RegExp} */
+        this.resourcePath = null;


The SvgSprite class already has this property. Why do you need to repeat it here also?

No we don't need to repeat this property. Will remove.

Actually we do need it, let me explain. Consider two webpack compilers (compiler A & B), each of them having it's own plugin instance:

The optimize hook associated with the compiler A is called, triggering sprite.generate

The sprite resourcePath changed from X to Y

The optimize hook associated with the compiler B is called, triggering sprite.generate

The sprite hasn't changed, so generate is a no-op

If we would be using the sprite's resource path to create the previousResourcePathRegExp, it would be wrong on the B compiler because it would actually be using the new one.

bensampaio · 2018-09-03T13:57:46Z

lib/SvgStorePluginSpriter.js

+        /** @member {?RegExp} */
+        this.previousResourcePathRegExp = null;
+        /** @member {SvgSprite} */
+        this.svgSprite = svgSprite;


Just call it sprite, since we don't have any other kind of sprites in this project and since this would be more consistent.

bensampaio · 2018-09-03T13:58:40Z

lib/SvgStorePluginSpriter.js

+/**
+ * SVG Store Plugin Spriter
+ */
+class SvgStorePluginSpriter {


What do you mean with this name? 😅 What's a spriter?

Quoting from some random dictionary in the web:

One involved in spriting; a designer of sprites (computer graphics).

I'm open to better names but I couldn't find any. Suggestions? 🤣

bensampaio · 2018-09-03T14:16:22Z

lib/SvgStorePluginSpriter.js

+     */
+    constructor(svgSprite) {
+        /** @member {?string} */
+        this.content = null;


Is it really a good idea to keep this information in memory? We are keeping lots of references to the SVG content. If a project has lots of icons this might not perform very well...

I wouldn't be worried about it as we are only storing the current and the previous strings. Also note that JS strings are by reference, which means that any variables pointing to the previous sprite contents would be pointing to the same memory and not a new one.

If you are still worried about it, we can instead compute a hash (md5 or sha) of the contents and store that instead. The hash is much smaller of course.

satazor · 2018-09-03T19:00:04Z

I am a bit surprised with these changes... I am not sure I get all the changes you made so I left some comments. I am missing some documentation in the code explaining why certain changes are necessary.

I answered the comments you made. I've tried to explain the reason for the changes in the PR description that I've updated 3 days ago. Feel free to ask whatever you like so that you understand why I made these changes.

I didn't have the time to properly evaluate the changes in SvgStorePlugin this time since they are quite extensive. Besides that, I noticed that you tried to use a deprecated variable so I would like first to know if there is a better alternative.

The changes on SvgStorePlugin are actually small. They are big because of a refactor were I moved a lot of code from it to the SvgStorePluginSpriter (state from SvgSprite was also moved there, which was related to the original issue). The reason of the refactor was that it was getting quite big.

Regarding the usage of the deprecated variable, let me think on a solution.

I also have doubts some of these changes are really scalable. Storing the content of icons, sprites and old sprites in memory doesn't really sound a very good idea... Or is this how other plugins implement HMR?

Storing of the previous sprite contents is very specific to this plugin, simply because this plugin does a expensive aggregation work (takes the "input" of multiple loaders and combines into an asset) that shouldn't be done between recompilations. Therefore, we need some way to check if that expensive work is necessary or not. Neverthess, I proposed a way to resolve the memory issue you pointed out.

satazor · 2018-09-04T01:53:25Z

I've changed the strategy so that SvgSprite doesn't remember the content nor interpolates the resourcePath (which makes dirty unnecessary as well). All of this logic was moved to the SvgStorePluginSpriter. Because of this, we no longer need to access the deprecated _compiler and also we don't need to store the previous content, which addresses your memory concerns.

Please review the recent changes and give me feedback 🍡

The changes on bensampaio#47 were causing problems when using multiple plugin instances. It's pretty common to have at least two plugin instances in the same node process in isomorphic projects. The reason was that SvgSprite's were storing state between compilations, such as the `dirty` and `updated` flags, causing inconsistencies when different compilations were running concurrently. To solve this, every plugin instance has its own compilation state, thanks to the SvgStorePluginSpriter. Note that SvgSprite instances are still shared whenever possible. Additional changes: - Allow partial options to be passed to the plugin - Moved the options parameter from SvgSprite's generate method to the constructor because it is a property that changes the content; this way the `dirty` and `updated` flag are always correct - Update example app scripts and add a start:prod script - Fix replacement of interpolated paths for modules of type CssModule - Fix errors being swalled in the recently added MissingDimensionsException Fixes bensampaio#53

satazor · 2018-09-10T15:26:45Z

@bensampaio did you take the time to evaluate the recent changes? It should be a lot simpler now and your concerns were addressed.

bensampaio · 2018-09-10T20:31:41Z

@satazor I saw the changes, thanks! I already started a review but didn't have time to finish, so no pressure please 😉

In any case, I want to let you know that this time I want to do things differently. I need to assure the quality of this loader / plugin since I am also using it at work. So this time I will test this myself since the tests for the previous PR should have detected this kind of problems. The plugin is not only meant for development so it should always be tested both for development and production. Since this will take quite some time I am also considering to remove this feature from version 4 and bring it back on a minor version.

I'll try to finish the review this week and I will let you know how I want to proceed with these changes.

satazor · 2018-09-10T23:56:38Z

@bensampaio alright. Please note that I've tested in both development and production but the bug only manifested on isomorphic projects in production (two instances of the plugin on the same process). Unfortunately, that's why I haven't detected it in my own testing because I've used a simple SPA project and the example app.

Moreover, when you do the testing, HMR for CSS files do not work due to a missing feature in mini-css-extract-plugin: webpack-contrib/mini-css-extract-plugin#34.

bensampaio · 2018-09-13T14:16:16Z

@satazor I understand that. For that reason, I added more SVGs to the example app and added multiple sprites to the webpack config. I also added some of the changes you made in this MR to the example app so that I could test it more extensively. Besides that, I also upgraded babel to v7. Could you rebase your changes on master and then let me know if it is possible to reproduce the problem you found with the changes I made to the example app?

If yes, please let me know the steps.
If not, please let me know what's missing.

Thanks for the hard work!

satazor · 2018-09-14T10:30:40Z

@bensampaio Thanks for taking the time to tackle this. I think you misunderstood the problem unfortunately. The issue is not when multiple sprites are used. The issue manifests when two webpack compilers are running concurrently and with different configs, each of them having their own SvgStorePlugin instances (new SvgStorePlugin()). More specifically, the changed flag of the shared SvgSprite instances becomes inconsistent in this scenario and, because of that, the interpolation was not being done for the compiler that ended last.

The example app only has one instance of the SvgStorePlugin and only one compilation occurring in parallel. To reproduce the issue, you can test it in our isomorphic boilerplate that compiles the client and server bundles:

Clone https://github.com/moxystudio/react-with-moxy
Run npm install inside it
Run npm link external-svg-sprite-loader (link it before)
Run npm run build to build a production version
Run npm start to test the production build
Open the network tab and you should see some requests failing

You may test my branch to see if my changes work correctly. If necessary, we can do a quick call so that I can explain you in detail.

Thank you for porting some of my changes. You might want to port this change as well: https://github.com/karify/external-svg-sprite-loader/pull/52/files#diff-1918bc9ded041b59f4a3b516dea6d487R70. Basically, you forgot to rethrow the error there.

satazor · 2018-09-14T10:32:47Z

To replicate the issue here in the project, we could create another example app using nextjs. Let me know what you think.

bensampaio · 2018-09-14T12:53:08Z

@satazor Ah I think I get it now. So if I add server side rendering do the example app I should be able to reproduce the problem, right?

bensampaio · 2018-09-14T12:56:06Z

Nice one with the other change you mentioned. I ported it as well. Thanks!

satazor · 2018-09-14T13:39:47Z

@bensampaio Yep, that’s correct. I suggested something like nextjs but it can be hand made, but might take some time to setup it correctly.

bensampaio · 2018-09-14T13:50:44Z

@satazor I have the same setup at work so it didn't take that long. I believe I replicated the issue. Can you have a look at the changes I just pushed to master? The production build now fails because the sprites are not being emitted for the client build. Is this related, or is it something else?

satazor · 2018-09-14T13:54:38Z

It’s related because the changed flag was false for one of the compilers, so the emit was skipped, along with other hooks as well.

satazor · 2018-09-14T13:56:13Z

Sorry for the short answers but I’m responding on my phone.

satazor · 2018-09-15T18:39:31Z

@bensampaio the changes on master look good!

Now that we can replicate the issue, what do you think of the solution that I proposed in this PR? Essentially, this PR moves all the "stateful" flags to the SvgStorePluginSpriter instead. Initially, I moved to SvgStorePlugin but it was growing considerably with all the new stateful flags. That's the reason why I created SvgStorePluginSpriter.

osvarychevskyi · 2018-09-24T15:14:38Z

Hi guys,

I've just installed this plugin(latest beta version) and I've experienced the similar issue with a shared state when using webpack multiple compiler.

I could not find how to fix it. But I've found this plugin external-svg-symbols-loader, which is an obvious copy of yours, with modifications. It has no issue with shared state but it has an issue in index.js with iconMetaData. So I've replaced it with yours functionality and it worked out.

Could you have a look in it and just copy some major differences that break the state.

It would be nice to fix this issue as your plugin is pretty usefull.

bensampaio · 2018-09-24T15:34:19Z

@satazor and @osvarychevskyi I am sorry for the delay on this. I want to continue with this but the last few weeks have been pretty busy at work and at home. Mainly because, I'll be on holidays for 3 weeks starting next week. I will try to finish the review this week. If I don't then you know why. In any case, don't worry, this won't be forgotten :)

bensampaio · 2018-10-24T08:17:58Z

Closed in favor of #54

satazor changed the title ~~fix: fix sprite paths not being correctly replaced in production~~ Fix sprite paths not being correctly replaced in production Aug 29, 2018

satazor force-pushed the fix-prod-usage branch 4 times, most recently from 7f81395 to 55f0bd1 Compare August 29, 2018 20:19

satazor force-pushed the fix-prod-usage branch 7 times, most recently from 65c0a44 to 383c9eb Compare August 30, 2018 16:22

satazor changed the title ~~Fix sprite paths not being correctly replaced in production~~ Do not share compilation state between plugin instances Aug 30, 2018

satazor force-pushed the fix-prod-usage branch from 383c9eb to 679fe27 Compare August 30, 2018 16:26

satazor force-pushed the fix-prod-usage branch from 679fe27 to c2d8c3c Compare August 30, 2018 16:29

satazor force-pushed the fix-prod-usage branch from c2d8c3c to 0f3bf04 Compare August 30, 2018 16:48

satazor force-pushed the fix-prod-usage branch 8 times, most recently from e1ee358 to 84f035d Compare August 30, 2018 19:58

satazor force-pushed the fix-prod-usage branch 2 times, most recently from 982a294 to 8b4c2a9 Compare August 31, 2018 01:50

bensampaio requested changes Sep 3, 2018

View reviewed changes

satazor force-pushed the fix-prod-usage branch 2 times, most recently from 44bfdda to 37b2783 Compare September 4, 2018 01:49

satazor force-pushed the fix-prod-usage branch 2 times, most recently from ee98a39 to 3c8d410 Compare September 4, 2018 02:01

satazor force-pushed the fix-prod-usage branch from 3c8d410 to b9ddbe8 Compare September 4, 2018 02:57

Merge branch 'master' into fix-prod-usage

b8da0b9

bensampaio mentioned this pull request Oct 23, 2018

Change icon id on add to sprite #48

Closed

bensampaio closed this Oct 24, 2018

Do not share compilation state between plugin instances #52

Do not share compilation state between plugin instances #52

Conversation

satazor commented Aug 29, 2018 • edited

satazor commented Aug 29, 2018

satazor commented Aug 30, 2018 • edited

bensampaio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satazor Sep 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satazor Sep 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satazor Sep 3, 2018 • edited

Choose a reason for hiding this comment

satazor commented Sep 3, 2018 • edited

satazor commented Sep 4, 2018 • edited

satazor commented Sep 10, 2018

bensampaio commented Sep 10, 2018

satazor commented Sep 10, 2018 • edited

bensampaio commented Sep 13, 2018 • edited

satazor commented Sep 14, 2018 • edited

satazor commented Sep 14, 2018

bensampaio commented Sep 14, 2018

bensampaio commented Sep 14, 2018

satazor commented Sep 14, 2018

bensampaio commented Sep 14, 2018

satazor commented Sep 14, 2018 • edited

satazor commented Sep 14, 2018

satazor commented Sep 15, 2018

osvarychevskyi commented Sep 24, 2018 • edited

bensampaio commented Sep 24, 2018

bensampaio commented Oct 24, 2018

satazor commented Aug 29, 2018 •

edited

satazor commented Aug 30, 2018 •

edited

satazor Sep 3, 2018 •

edited

satazor Sep 3, 2018 •

edited

satazor Sep 3, 2018 •

edited

satazor commented Sep 3, 2018 •

edited

satazor commented Sep 4, 2018 •

edited

satazor commented Sep 10, 2018 •

edited

bensampaio commented Sep 13, 2018 •

edited

satazor commented Sep 14, 2018 •

edited

satazor commented Sep 14, 2018 •

edited

osvarychevskyi commented Sep 24, 2018 •

edited