Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Feb 13, 2019
Merged

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

merged 2 commits into from
Feb 13, 2019

Conversation

svlapin
Copy link
Contributor

@svlapin svlapin commented Feb 11, 2019

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
Copy link
Contributor

@pmuens pmuens left a comment

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
Copy link
Contributor Author

svlapin commented Feb 13, 2019

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

@pmuens pmuens pmuens approved these changes

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