Skip to content
This repository has been archived by the owner on Oct 10, 2022. It is now read-only.

Improve defaultFilter #226

Merged
merged 1 commit into from
Dec 15, 2020
Merged

Improve defaultFilter #226

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 12 additions & 11 deletions src/deploy/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,20 @@
const path = require('path')
const { basename, sep } = require('path')

const pWaitFor = require('p-wait-for')

// Default filter when scanning for files
const defaultFilter = (filePath) => {
if (filePath == null) return false
const filename = path.basename(filePath)
switch (true) {
case filename === 'node_modules':
case filename.startsWith('.') && filename !== '.well-known':
case filename.match(/(\/__MACOSX|\/\.)/):
return false
default:
return true
if (filePath == null) {
return false
}

const filename = basename(filePath)
return (
filename !== 'node_modules' &&
!(filename.startsWith('.') && filename !== '.well-known') &&
!filename.includes('/__MACOSX') &&
Copy link
Contributor Author

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.

Copy link
Contributor

@erezrokah erezrokah Dec 13, 2020 •
edited
Loading

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.

Copy link
Contributor Author

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.

!filename.includes('/.')
)
}

// normalize windows paths to unix paths
Expand All @@ -23,7 +24,7 @@ const normalizePath = (relname) => {
}
return (
relname
.split(path.sep)
.split(sep)
// .map(segment => encodeURI(segment)) // TODO I'm fairly certain we shouldn't encodeURI here, thats only for the file upload step
.join('/')
)
Expand Down
44 changes: 11 additions & 33 deletions src/deploy/util.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,38 +16,16 @@ test('normalizePath should throw the error if name is invalid', (t) => {
t.throws(() => normalizePath('#'))
})

test('pass empty name to defaultFilter', (t) => {
const cases = [
{
input: null,
expect: false,
},
{
input: undefined,
expect: false,
},
{
input: 'foo/bar/baz.js',
expect: true,
},
{
input: 'directory/node_modules',
expect: false,
},
{
input: 'directory/.gitignore',
expect: false,
},
{
input: 'directory/.well-known',
expect: true,
},
{
input: '__MACOSX',
expect: true,
},
]
cases.forEach(({ input, expect }) => {
t.is(defaultFilter(input), expect)
const filteredFiles = ['foo/bar/baz.js', 'directory/.well-known', '__MACOSX']
filteredFiles.forEach((filePath) => {
test(`filters ${filePath}`, (t) => {
t.true(defaultFilter(filePath))
})
})

const unfilteredFiles = [null, undefined, 'directory/node_modules', 'directory/.gitignore']
unfilteredFiles.forEach((filePath) => {
test(`does not filter ${filePath}`, (t) => {
t.false(defaultFilter(filePath))
})
})