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 packaging with files that contain dots as part of the name #8130

Merged
merged 2 commits into from Aug 27, 2020

Conversation

crash7
Copy link
Contributor

@crash7 crash7 commented Aug 25, 2020

This ports a fix introduced to serverless-components: serverless-components/aws-lambda#26 but given some changes in the test, it probably needs to be flagged as a breaking change.

Closes: #8125

@codecov-commenter
Copy link

codecov-commenter commented Aug 25, 2020

Codecov Report

Merging #8130 into v2 will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##               v2    #8130   +/-   ##
=======================================
  Coverage   88.28%   88.28%           
=======================================
  Files         248      248           
  Lines        9432     9432           
=======================================
  Hits         8327     8327           
  Misses       1105     1105           
Impacted Files Coverage Δ
lib/plugins/package/lib/packageService.js 86.06% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45080ed...42de6cc. Read the comment docs.

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

but given some changes in the test, it probably needs to be flagged as a breaking change.

@crash7 change looks good to me, still I don't think it's breaking. I believe files with dots in name were expected to be included, and it was a bug they weren't

Did you have any other concern in mind pointing that it should be marked as breaking?

@crash7
Copy link
Contributor Author

crash7 commented Aug 25, 2020

Did you have any other concern in mind pointing that it should be marked as breaking?

@medikoo Just pointed that because codebases using just ** to exclude all files won't work the same way after this change:

package:
  exclude:
    - '**'
  include:
    - src/handle.js

This fill include files with dots and also the handler file.

Those codebases will need to change the "exclude all" rule to **/* to make sure those dotted files are not included.

However I see what you mean, those files should have been included initially and they doesn't due to this bug, so maybe it's fine.

Thanks!

@medikoo
Copy link
Contributor

medikoo commented Aug 25, 2020

@crash7 thanks for explanation.

Do you understand why handling of ** and **/* differs? Is it globby package resolution quirk, or is this how global pattern handling is specified (if there's any specification) ?

@crash7
Copy link
Contributor Author

crash7 commented Aug 25, 2020

@medikoo while trying to find a good answer to your question, I ended doing a lot of digging and finding something else.

Short version:
Let's try to upgrade to globby@v11, seems that one of its dependencies (picomatch) follows the globstar spec correctly.

Long version:
This problem affects globby@v9.2.0, which is the version serverless uses right now. Upgrading to globby@>v10 seems to solve the problem (need to run all the tests)

That version of globby upgraded the lib that uses under the hood: fast-glob to v3. Testing this library in particular confirms that lower versions (specifically fast-glob@v2.2.7, which is the one that globby@v9.2.0 uses) produces the same error with ** but works fine with **/*.

In fact, fast-glob@v3 introduced a lot of breaking changes, one of them being the upgrade of micromatch@v3 to micromatch@v4. Testing micromatch@v3 produces the same behavior.

Unfortunately, the changelog for micromatch@v4 doesn't tell me nothing.

On the other hand, micromatch@v2 works fine too, so the problem was introduced in v3 and then solved again in v4. Checking the code closely, v3 added nanomatch but v4 removed it (and yes, nanomatch@latest produces the same behavior) and switched to picomatch witch is the one that apparently implements globstar correctly and works correctly just using **.

Hope you are not bored after reading all of this 😂

@crash7
Copy link
Contributor Author

crash7 commented Aug 25, 2020

@medikoo
I updated the code to use latest version of globby, the breaking changes introduced in both v10 and v11 are related with libraries or other changes that doesn't have an impact in the API. I also found the we use nanomatch directly, which based in my previous investigation has a bug with files with dots, I replaced it with micromatch, which is the same library that globby is using right now. The API is the same as nanomatch, so the only change is the name of the function.

UPDATE: ok, I can't update globby until serverless drops support for node 6 :( - just rolled back my changes.

Also found that one of the tests in my initial commit was wrong, so I fixed that too.

Let me know what you think, thanks!

@medikoo
Copy link
Contributor

medikoo commented Aug 26, 2020

@crash7 if you feel that upgrading globby to v11 will fix it. Feel free to do it, and rebase the PR against v2 branch.

We're planning to release v2 next week.

That'll probably be best approach to that (as anyway with v2 we plan to upgrade all dependencies)

@crash7
Copy link
Contributor Author

crash7 commented Aug 26, 2020

@medikoo upgrading globby and replacing nanomatch fixed it (e297d82) but the CI check for node6 failed, that's why I reverted the changes.

I don't mind rebasing this against v2 if you are planning to release the stable version soon, as long as I can use a next version in my project, I don't care for now.

This ports a fix introduced in serverless-components: serverless-components/aws-lambda#26 but given some changes in the test,
it probably needs to be flagged as a breaking change.

Fixes serverless#8125
@medikoo
Copy link
Contributor

medikoo commented Aug 26, 2020

@medikoo upgrading globby and replacing nanomatch fixed it (e297d82) but the CI check for node6 failed, that's why I reverted the changes.

That's clear. In v1 we need to maintain support for Node.js v6, so it's a no go. With v2 we drop support for Node.js v6 and v8 and branch has already a CI update to test against those versions.

I don't mind rebasing this against v2 if you are planning to release the stable version soon, as long as I can use a next version in my project, I don't care for now.

It'll be released next week, and it contains few minor (but breaking changes). Check the milestone that outlines them: https://github.com/serverless/serverless/milestone/82

If you do not see any deprecation logs during deployment it means you'll be able to upgrade right away

@crash7 crash7 changed the base branch from master to v2 August 26, 2020 14:49
@crash7
Copy link
Contributor Author

crash7 commented Aug 26, 2020

@medikoo just rebased with v2 and changed the base for this PR - all node tests passed :)

It'll be released next week, and it contains few minor (but breaking changes). Check the milestone that outlines them: https://github.com/serverless/serverless/milestone/82

Let me know if you need a hand with something from that milestone.

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

Thank you!

@medikoo
Copy link
Contributor

medikoo commented Aug 27, 2020

Let me know if you need a hand with something from that milestone.

@crash7 We appreciate help with any marked by help wanted label, here: https://github.com/serverless/serverless/milestone/82

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Files that contain dots are ignored during packaging
3 participants