Skip to content

Fix: override wildcard glob pattern (**) in resolveFilePathsFromPatterns#5825

Merged
pmuens merged 2 commits into
serverless:masterfrom
svlapin:fix/sl-override-wildcard-glob
Feb 13, 2019
Merged

Fix: override wildcard glob pattern (**) in resolveFilePathsFromPatterns#5825
pmuens merged 2 commits into
serverless:masterfrom
svlapin:fix/sl-override-wildcard-glob

Conversation

@svlapin

@svlapin svlapin commented Feb 11, 2019

Copy link
Copy Markdown
Contributor

What did you implement:

Ability to override wildcard glob pattern by a negative params.include to avoid unnecessary traversal and get much faster glob matching during packaging in some cases.

When globby is provided with an array of patterns, it adds all subsequent negative patterns as ignore option of a particular task (source code here). However, that is not useful when the wildcard comes the last (as it is now) - ignore for the relevant matching task is always empty.

Putting the least specific pattern first makes it possible to override it with a subsequent negative one and avoid unnecessary traversal in e.g. node_modules, .git, etc.

Related to #4263, #5574.

How did you implement it:

Put wildcard pattern ** first in resolveFilePathsFromPatterns.

How can we verify it:

Minimal example

Without this PR:

svl@sergeys-laptop:~/dev/serverless-package-example$ time ./node_modules/.bin/serverless package 
Serverless: Packaging service...

real	1m28,677s
user	1m32,756s
sys	0m1,412s

With this PR:

svl@sergeys-laptop:~/dev/serverless-package-example$ time ./node_modules/.bin/serverless package 
Serverless: Packaging service...

real	0m0,898s
user	0m1,017s
sys	0m0,090s

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@pmuens pmuens self-assigned this Feb 12, 2019
@pmuens pmuens changed the title fix: override wildcard glob pattern (**) in resolveFilePathsFromPatterns Fix: override wildcard glob pattern (**) in resolveFilePathsFromPatterns Feb 13, 2019
pmuens
pmuens approved these changes Feb 13, 2019
View reviewed changes

@pmuens pmuens left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! Thanks for taking a deeper dive into this and fixing it @svlapin 👍

I just tested it and it works great! Took more than 2 minutes vs. 2 1/2 seconds. This is awesome 👌

I just added a clarification comment to the code you changed so that others who might touch this won't change the order by accident. Also looked into potential test cases, but other than mocking globby there's no way to test this in a meaningful way...

Will merge once Travis agrees :shipit:

@pmuens pmuens added this to the 1.38.0 milestone Feb 13, 2019
@pmuens pmuens merged commit b6e63f0 into serverless:master Feb 13, 2019
@svlapin svlapin deleted the fix/sl-override-wildcard-glob branch February 13, 2019 15:18
@svlapin

svlapin commented Feb 13, 2019

Copy link
Copy Markdown
Contributor Author

Thanks for the review @pmuens 👍
Side note - actually I believe a better proper approach would be to take options.exclude into account while doing initial matching (like it was before https://github.com/serverless/serverless/pull/5574/files#diff-0333f308ebf44d187c45d0955bfe24c1R191) or rely on user-defined patterns only, but looks like it was intentionally avoided to speed up excludeDevDependencies somehow.

@bennbollay bennbollay mentioned this pull request Feb 26, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@pmuens pmuens pmuens approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@pmuens pmuens

Labels

None yet

Projects

None yet

Milestone

1.38.0

Development

Successfully merging this pull request may close these issues.

2 participants

@svlapin @pmuens