-
Notifications
You must be signed in to change notification settings - Fork 43
Conversation
30bfc01
to
9440834
Compare
return ( | ||
filename !== 'node_modules' && | ||
!(filename.startsWith('.') && filename !== '.well-known') && | ||
!filename.includes('/__MACOSX') && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic in lines 15
and 16
is kept the same as the original.
However, the original had a bug it seemed, since filename
cannot start with /
.
I am not 100% what the intent is here. My guess is:
- do not deploy
node_modules
- do not deploy any dot files, except
.well-known
- do not deploy
__MACOSX
I am not quite sure how the recursion applies. For example, if node_modules
is excluded, does this function gets called with node_modules/example-module/
? Depending on the answer to this question, the test would be different.
We might also consider using junk
.
@erezrokah What are your thoughts on all this?
It'd be nice to improve this, but I don't want to break anything either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See relevant CLI PR netlify/cli#1272 and the logic the js-client
is trying to mimic:
https://github.com/netlify/open-api/blob/e72e2eb2d7eedf02bccc5916fc0330022f7f823b/go/porcelain/deploy.go#L692
- do not deploy any dot files, except
.well-known
An important distinction is when the publish directory is hidden netlify/cli#1227 (comment)
I am not quite sure how the recursion applies
I think once a directory is excluded, so does its nested directories:
https://github.com/okdistribute/folder-walker/blob/9c3b07320e152def5c3cbc26c772ebebc6d5c124/index.js#L44
You're correct with the intent and the changes you made makes sense.
Not sure if we want to improve the fix from the CLI as a breaking change. The CLI has some special handling when deploying the root directory as described in the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a breaking change that we might want to hold on for the moment, especially before the holidays.
The initial intent was to improve the coding style, so let's merge this PR with no change of behavior, even if the code is somewhat nonsensical (since it replicates the original code), then follow-up with another PR for the behavior part.
9440834
to
c3b2105
Compare
Refactors default deploy file filter.