Skip to content
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.

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

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

Merged
merged 2 commits into from Feb 13, 2019

Conversation

Projects
2 participants
@svlapin
Copy link
Contributor

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 added this to In progress in Serverless via automation Feb 12, 2019

@pmuens pmuens added the pr/in-review label 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

Copy link
Member

pmuens left a comment

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:

Serverless automation moved this from In progress to Reviewer approved Feb 13, 2019

@pmuens pmuens added this to the 1.38.0 milestone Feb 13, 2019

@pmuens pmuens merged commit b6e63f0 into serverless:master Feb 13, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Serverless automation moved this from Reviewer approved to Done Feb 13, 2019

@svlapin svlapin deleted the svlapin:fix/sl-override-wildcard-glob branch Feb 13, 2019

@svlapin

This comment has been minimized.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.