Default the glob options follow & strict to true, with new options to override #1104

beatrizdemiguelperez · 2017-12-03T19:52:23Z

Description of what's changed/fixed.

Please ensure that gulp lint test passes locally prior to filing a PR!

googlebot · 2017-12-03T19:52:25Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

googlebot · 2017-12-04T08:51:09Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

coveralls · 2017-12-04T08:56:30Z

Coverage increased (+0.05%) to 84.573% when pulling d6030c3 on BBVAEngineering:glob-options-for-workbox-build into fb016ed on GoogleChrome:v3.

coveralls · 2017-12-04T09:13:20Z

Coverage decreased (-0.06%) to 84.467% when pulling 839b3b4 on BBVAEngineering:glob-options-for-workbox-build into fb016ed on GoogleChrome:v3.

googlebot · 2017-12-04T10:04:04Z

CLAs look good, thanks!

coveralls · 2017-12-04T10:09:15Z

Coverage decreased (-0.06%) to 84.467% when pulling 9a7db79 on BBVAEngineering:glob-options-for-workbox-build into fb016ed on GoogleChrome:v3.

googlebot · 2017-12-04T11:08:43Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

…when attempting to read a directory and follow symlinked directories when expanding ** patterns

coveralls · 2017-12-04T11:24:56Z

Coverage decreased (-0.008%) to 84.52% when pulling 3129073 on BBVAEngineering:glob-options-for-workbox-build into fb016ed on GoogleChrome:v3.

gauntface · 2017-12-04T19:09:47Z

packages/workbox-build/src/_types.js

+ * expanding ** patterns. Note that this can result in a lot of duplicate
+ * references in the presence of cyclic links.
+ *
+ * E.g. `true`


Can you remove the e.g. true piece.

gauntface · 2017-12-04T19:10:24Z

packages/workbox-build/src/_types.js

@@ -132,6 +132,19 @@ import './_version.mjs';
 *
 * E.g. `['**\/ignored.html']`
 *
+ * @property {Boolean} [globFollow=true] Follow symlinked directories when
+ * expanding ** patterns. Note that this can result in a lot of duplicate


Could you add single quotes around the patterns like: '**'

gauntface · 2017-12-04T19:10:57Z

packages/workbox-build/src/_types.js

+ * search of other matches. Set the strict option to raise an error in these
+ * cases.
+ *
+ * E.g. `true`


Please remove this - Our JSDocs will call out the default value.

gauntface · 2017-12-04T19:11:58Z

packages/workbox-build/src/lib/get-file-details.js

  let globbedFiles;
  try {
    globbedFiles = glob.sync(globPattern, {
      cwd: globDirectory,
      ignore: globIgnores,
+      follow: typeof globFollow !== 'undefined'? globFollow : true,


@jeffposnick you've been doing some work on the defaults, is this the best approach or should this be moved to a defaults value.

gauntface · 2017-12-04T19:17:25Z

cc @jeffposnick

Thanks for this @beatrizdemiguelperez, I've added some nit comments if you could take a look. One comment is more asking Jeff for comment who has been working on this a lot more closely than I have lately.

@adrigzr could you confirm you are all ok with this landing in Workbox (Just cos the weird Google license I have to worry about).

Out of interest, so it make sense to change the input to something like:

{
    globDirectory: ...,
    globPattern: ...,
    globOptions: {
        follow: ...,
        strict: ...,
        ignore: ...,
    }
}

or even:

{
    globPattern: ...,
    globOptions: {
        cwd: ...,
        follow: ...,
        strict: ...,
        ignore: ...,
    }
}

I'm just thinking that if we did this, we could arguably just pass in the glob options and support anything in the future without changes.

adrigzr · 2017-12-05T07:54:54Z

Hi @gauntface,

Thanks for the review. Yes, I'm working with @beatrizdemiguelperez on this feature. It's all ok. I've already signed the Google agreement.

About the change you suggest (globOptions), it was our first approach, but given that it wouldn't be backward compatible, we didn't make it that way.

On the other hand, as documentation suggests, setting follow: true on node-glob can lead to duplicate references in the presence of cyclic links. Are we sure we want this by default?

Thanks again for your time.

coveralls · 2017-12-05T08:02:08Z

Coverage decreased (-0.008%) to 84.52% when pulling bd8e932 on BBVAEngineering:glob-options-for-workbox-build into fb016ed on GoogleChrome:v3.

beatrizdemiguelperez · 2017-12-05T09:52:12Z

Yes, as @adrigzr said we want to be compatible with the previous version.
Workbox-build has already those globOptions {globDirectory, globIgnores, globPattern} separately so we think that is better to add two more instead of group all on "globOptions"

Again thanks :)

jeffposnick · 2017-12-05T15:58:58Z

Thanks for the contribution!

I'm of the opinion that the options which could be passed to glob.sync() are long enough and have the potential to change the output to the extent that we should continue validating them and only setting the ones we explicitly want to. So I'm fine with creating new top-level options for this functionality instead of just passing through a bag of options object directly.

I left a few inline comments, but my one request would be some additional changes to the tests under workbox-build/node/entry-points/.

There are tests there that check the behavior of the defaults set by joi's validation, which would need to be updated.
I'd feel better about the test coverage if there were additions to the end-to-end tests in that directory that used something like ensureSymlink to create a symlinked source directory, and tested the output with globFollow enabled and disabled.

If you're not comfortable or it's not clear how to best make changes to the test suite let me know, and I can push a commit with additional test cases.

adrigzr · 2017-12-07T12:02:59Z

Hi @jeffposnick,

Thanks for your answer. I've submitted the requested changes. Could you check the PR code again?

coveralls · 2017-12-07T12:06:45Z

Coverage increased (+0.2%) to 84.776% when pulling b317257 on BBVAEngineering:glob-options-for-workbox-build into fb016ed on GoogleChrome:v3.

jeffposnick · 2017-12-07T21:02:49Z

Thanks, @beatrizdemiguelperez & @adrigzr!

jeffposnick · 2017-12-07T22:03:30Z

I'll give @gauntface a little time to chime in in case he has other feedback, and if not, merge.

gauntface

LGTM thank you so much for raising this PR.

beatrizdemiguelperez mentioned this pull request Dec 3, 2017

Follow symbolic links when performing glob operations #1093

Closed

beatrizdemiguelperez and others added 4 commits December 4, 2017 12:16

[Workbox-Build] Raise an error when an unusual error is encountered …

0dbe0a4

…when attempting to read a directory and follow symlinked directories when expanding ** patterns

Change globFollows option to globFollow

9077a55

Add unit testing for globFollow and globStrict

9366e95

Update doc with globFollow and globStrict options

3129073

gauntface reviewed Dec 4, 2017

View reviewed changes

Update doc issues from pull-request comments

bd8e932

[test] Add test for globFollow option

b317257

jeffposnick approved these changes Dec 7, 2017

View reviewed changes

jeffposnick changed the title ~~#1093 issue [Workbox-Build] Raise an error when an unusual error is encountered …~~ Default the glob options follow & strict to true, with new options to override Dec 7, 2017

gauntface approved these changes Dec 8, 2017

View reviewed changes

gauntface merged commit b646c1f into GoogleChrome:v3 Dec 8, 2017

dependabot-preview bot mentioned this pull request Mar 19, 2018

Bump workbox-webpack-plugin from 2.1.3 to 3.0.0 straw-hat-labs/phoenix_assets_webpack#12

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default the glob options follow & strict to true, with new options to override #1104

Default the glob options follow & strict to true, with new options to override #1104

beatrizdemiguelperez commented Dec 3, 2017 •

edited by jeffposnick

Loading

googlebot commented Dec 3, 2017

googlebot commented Dec 4, 2017

coveralls commented Dec 4, 2017

coveralls commented Dec 4, 2017

googlebot commented Dec 4, 2017

coveralls commented Dec 4, 2017

googlebot commented Dec 4, 2017

coveralls commented Dec 4, 2017

gauntface Dec 4, 2017

gauntface Dec 4, 2017

gauntface Dec 4, 2017

gauntface Dec 4, 2017

gauntface commented Dec 4, 2017

adrigzr commented Dec 5, 2017

coveralls commented Dec 5, 2017

beatrizdemiguelperez commented Dec 5, 2017 •

edited

Loading

jeffposnick commented Dec 5, 2017

adrigzr commented Dec 7, 2017

coveralls commented Dec 7, 2017

jeffposnick commented Dec 7, 2017

jeffposnick commented Dec 7, 2017

gauntface left a comment

Default the glob options follow & strict to true, with new options to override #1104

Default the glob options follow & strict to true, with new options to override #1104

Conversation

beatrizdemiguelperez commented Dec 3, 2017 • edited by jeffposnick Loading

googlebot commented Dec 3, 2017

googlebot commented Dec 4, 2017

coveralls commented Dec 4, 2017

coveralls commented Dec 4, 2017

googlebot commented Dec 4, 2017

coveralls commented Dec 4, 2017

googlebot commented Dec 4, 2017

coveralls commented Dec 4, 2017

gauntface Dec 4, 2017

Choose a reason for hiding this comment

gauntface Dec 4, 2017

Choose a reason for hiding this comment

gauntface Dec 4, 2017

Choose a reason for hiding this comment

gauntface Dec 4, 2017

Choose a reason for hiding this comment

gauntface commented Dec 4, 2017

adrigzr commented Dec 5, 2017

coveralls commented Dec 5, 2017

beatrizdemiguelperez commented Dec 5, 2017 • edited Loading

jeffposnick commented Dec 5, 2017

adrigzr commented Dec 7, 2017

coveralls commented Dec 7, 2017

jeffposnick commented Dec 7, 2017

jeffposnick commented Dec 7, 2017

gauntface left a comment

Choose a reason for hiding this comment

beatrizdemiguelperez commented Dec 3, 2017 •

edited by jeffposnick

Loading

beatrizdemiguelperez commented Dec 5, 2017 •

edited

Loading