Add `cacheDir` option to plugin. #11

sgomes · 2019-06-14T18:02:24Z

Hello again, @gilmoreorless ! How have you been? 🙂

We've run into an interesting issue with moment-timezone-data-webpack-plugin in our setup.

We have two separate builds, targeting older and newer browsers, and we used to run them in parallel, on the same server. This was leading to a race condition which caused some builds to break. We tracked it down to the moment-timezone-data-webpack-plugin cache; in some cases, one of the builds would read the directory in an inconsistent state, while the other build was writing to it.

We made the builds sequential after that, which solved the issue, but we'd go like to go back to running them in parallel for performance reasons. In order to do that, I propose adding a new cacheDir option with this PR.

The new option allows a user to specify an absolute path where the cached files will be stored. If not provided, the default auto-generated path will be used instead, as before.

In our case, we'd set it to different values in each build, and thus avoid any collisions. This would solve the problem for us, but I'd love your thoughts on whether it's an appropriate addition to moment-timezone-data-webpack-plugin.

Thank you again for an exceptionally useful plugin! 👍

The new option allows a user to specify an absolute path where the cached files will be stored. If not provided, the default auto-generated path will be used instead, as before.

gilmoreorless

Hi @sgomes, thanks for another useful contribution. 😃

That's a doozy of a use case you've found, and certainly not one I'd thought of. I wonder if this has this happened with other webpack plugins/loaders that cache output, as I took the find-cache-dir strategy from things like babel-loader.

I'd love your thoughts on whether it's an appropriate addition

Funnily enough, an option called exactly cacheDir was in my first list of ideas when planning this plugin. The build servers I was dealing with at the time ran the npm install and webpack build processes as separate users in Docker containers, so trying to write a directory to node_modules from webpack caused a failure. I thought about adding a cacheDir option, but then solved the problem with the "fallback to a temp directory" approach it currently uses. I thought it might be overkill to add an option that possibly only I'd use. Turns out I should have put it in after all! 😄

Overall I like your solution, but there are some minor things I'd like fixed up before I can merge it.

gilmoreorless · 2019-06-16T09:48:37Z

README.md

@@ -115,6 +115,7 @@ There are three available options to filter the time zone data. **At least one o
  * _string_ — Include only this zone name as an exact match (e.g. `'Australia/Sydney'`).
  * _regexp_ — Include zones with names matching the regular expression (e.g. `/^Australia\//`).
  * _array_ (of the above types) — Include zones matching any of the values of the array. Each value can be a string or a regular expression, which will be matched following the rules above.
+* `cacheDir` _(string)_ — An absolute path where the generated files will be cached. If not provided, the files will be cached in an automatically-generated location.


There's a line above the list that says at least 1 of 3 options for filtering the data must be provided. That's still correct, and cacheDir is a separate concern from data filtering. Maybe it should be in a separate list on its own? Either that, or the intro line of the section should be reworded.

I'm not 100% sure on which direction to take, so I won't mark that as a blocker for merging the PR.

I created two lists; it seems expandable in the future, and it should be clear enough. Let me know if you want any other wording changes!

gilmoreorless · 2019-06-16T09:51:37Z

src/index.js

@@ -47,7 +48,7 @@ function throwInvalid(message) {
 }

 function validateOptions(options) {
-  const knownOptions = ['matchZones', 'startYear', 'endYear'];
+  const knownOptions = ['matchZones', 'startYear', 'endYear', 'cacheDir'];


This is going to change the semantics of a check later on, where at least one filtering option should be provided. Providing cacheDir and nothing else will pass the "at least one" check, but fail to filter the data.

Ah yes, good point. I missed that logic.

I'll separate filtering options from other options 👍

gilmoreorless · 2019-06-16T12:12:23Z

src/index.js

@@ -74,6 +75,11 @@ function validateOptions(options) {
      throwInvalid(`Invalid option — ${option} must be an integer.`);
    }
  });
+
+  // Invalid cache dir (not an absolute path)
+  if ('cacheDir' in options && !path.isAbsolute(options.cacheDir)) {


Out of curiosity, why does it have to be an absolute path?

It was mostly an attempt at making things more explicit; a relative path could be confusing depending on your build setup. But the more I think about it, the less it makes sense; developers can still use absolute paths if they want, and if they want to use relative ones it's on them to figure out what the base dir is.

I'll remove this check and just use a path.parse to do basic validation on the path.

gilmoreorless · 2019-06-16T12:13:32Z

src/index.js

+
+  // Invalid cache dir (not an absolute path)
+  if ('cacheDir' in options && !path.isAbsolute(options.cacheDir)) {
+    throwInvalid(`Provided cacheDir is not absolute: '${cacheDir}'`);


This line is causing a build failure due to a linting error: error 'cacheDir' is not defined

Good catch!

sgomes · 2019-06-17T10:08:36Z

Hi @sgomes, thanks for another useful contribution. 😃

Glad to help! 😁

That's a doozy of a use case you've found, and certainly not one I'd thought of. I wonder if this has this happened with other webpack plugins/loaders that cache output, as I took the find-cache-dir strategy from things like babel-loader.

It's quite possible we'll find similar race conditions elsewhere once we apply the changes to this plugin and re-enable parallel builds, yup. It does look like one of those situations where one error could be hiding others down the line. And the fact that it's non-deterministic makes it all harder.

Funnily enough, an option called exactly cacheDir was in my first list of ideas when planning this plugin. The build servers I was dealing with at the time ran the npm install and webpack build processes as separate users in Docker containers, so trying to write a directory to node_modules from webpack caused a failure. I thought about adding a cacheDir option, but then solved the problem with the "fallback to a temp directory" approach it currently uses. I thought it might be overkill to add an option that possibly only I'd use. Turns out I should have put it in after all! 😄

Hah, well, I'm not surprised you opted against adding it; it does seem like something not a lot of folks would be interested in. That's why I was unsure you'd want to add it to the plugin, too 🙂

Overall I like your solution, but there are some minor things I'd like fixed up before I can merge it.

No problem! I think I've addressed all the comments, but let me know if you'd like any further changes!

gilmoreorless · 2019-06-17T11:30:44Z

Excellent, looks good to me!

gilmoreorless · 2019-06-17T11:42:13Z

This is now available on npm as v1.1.0

sgomes · 2019-06-17T11:43:08Z

This is now available on npm as v1.1.0

Wow, that was super fast! Thank you so much, @gilmoreorless ! 🎉

gilmoreorless · 2019-06-17T11:44:18Z

You caught me at the right time. 😀

Add cacheDir option to plugin.

89691c5

The new option allows a user to specify an absolute path where the cached files will be stored. If not provided, the default auto-generated path will be used instead, as before.

gilmoreorless requested changes Jun 16, 2019

View reviewed changes

sgomes added 3 commits June 17, 2019 10:46

Fix filtering options validation logic.

d5337b5

Remove absolute path restriction on cacheDir.

25777ed

Distinguish between option types in README.

ae6707e

gilmoreorless approved these changes Jun 17, 2019

View reviewed changes

gilmoreorless merged commit a07fae6 into gilmoreorless:master Jun 17, 2019

sgomes deleted the add-cacheDir-option branch June 17, 2019 11:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `cacheDir` option to plugin. #11

Add `cacheDir` option to plugin. #11

sgomes commented Jun 14, 2019

gilmoreorless left a comment

gilmoreorless Jun 16, 2019

sgomes Jun 17, 2019

gilmoreorless Jun 16, 2019

sgomes Jun 17, 2019 •

edited

Loading

gilmoreorless Jun 16, 2019

sgomes Jun 17, 2019

gilmoreorless Jun 16, 2019

sgomes Jun 17, 2019

sgomes commented Jun 17, 2019 •

edited

Loading

gilmoreorless commented Jun 17, 2019

gilmoreorless commented Jun 17, 2019

sgomes commented Jun 17, 2019

gilmoreorless commented Jun 17, 2019

Add cacheDir option to plugin. #11

Add cacheDir option to plugin. #11

Conversation

sgomes commented Jun 14, 2019

gilmoreorless left a comment

Choose a reason for hiding this comment

gilmoreorless Jun 16, 2019

Choose a reason for hiding this comment

sgomes Jun 17, 2019

Choose a reason for hiding this comment

gilmoreorless Jun 16, 2019

Choose a reason for hiding this comment

sgomes Jun 17, 2019 • edited Loading

Choose a reason for hiding this comment

gilmoreorless Jun 16, 2019

Choose a reason for hiding this comment

sgomes Jun 17, 2019

Choose a reason for hiding this comment

gilmoreorless Jun 16, 2019

Choose a reason for hiding this comment

sgomes Jun 17, 2019

Choose a reason for hiding this comment

sgomes commented Jun 17, 2019 • edited Loading

gilmoreorless commented Jun 17, 2019

gilmoreorless commented Jun 17, 2019

sgomes commented Jun 17, 2019

gilmoreorless commented Jun 17, 2019

Add `cacheDir` option to plugin. #11

Add `cacheDir` option to plugin. #11

sgomes Jun 17, 2019 •

edited

Loading

sgomes commented Jun 17, 2019 •

edited

Loading