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.

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

fix: stop search if files names are undefined #6761

Merged
merged 6 commits into from
Aug 30, 2018
Merged

fix: stop search if files names are undefined #6761

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d56dcf3
stop search if is undefined
emuraton Jul 26, 2018
0f79b06
fix: callback early is readdir throw an error
emuraton Aug 24, 2018
f92bf6d
update changelog
emuraton Aug 29, 2018
edcfe78
Merge branch 'master' into crawlers-error-window
thymikee Aug 29, 2018
8f6488b
Merge branch 'master' into crawlers-error-window
emuraton Aug 30, 2018
fdf4e25
Merge branch 'master' into crawlers-error-window
thymikee Aug 30, 2018
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
18 changes: 18 additions & 0 deletions packages/jest-haste-map/src/crawlers/__tests__/node.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ jest.mock('fs', () => {
setTimeout(() => callback(null, ['directory', 'tomato.js']), 0);
} else if (dir === '/fruits/directory') {
setTimeout(() => callback(null, ['strawberry.js']), 0);
} else if (dir == '/empty') {
setTimeout(() => callback(null, null), 0);
}
}),
stat: jest.fn(stat),
Expand Down Expand Up @@ -217,4 +219,20 @@ describe('node crawler', () => {
expect(data.files).toEqual({});
});
});

it('completes with undefined names', () => {
process.platform = 'win32';

nodeCrawl = require('../node');

const files = Object.create(null);
return nodeCrawl({
data: {files},
extensions: ['js'],
ignore: pearMatcher,
roots: ['/empty'],
}).then(data => {
expect(data.files).toEqual({});
});
});
});
5 changes: 4 additions & 1 deletion packages/jest-haste-map/src/crawlers/node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,10 @@ function find(
activeCalls++;
fs.readdir(directory, (err, names) => {
activeCalls--;

if (!names) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about going a bit more idiomatic Node.js with if (err) {?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, if that's what happens (unhandled error) that's the correct fix. @emuraton do you have a reproduction case for your error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SimenB I did not suceed to replicate the issue case of @timbault.
But from the stack trace, callback doesnt return an error @thymikee, just names undefined.
I can add both check if you think its better (if( err || !names))

Doest it make sense?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it returns undefined names, but null err, it's a bug in Node imho and you should also file it there

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, the docs says you should always get an array: https://nodejs.org/api/fs.html#fs_fs_readdir_path_options_callback

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the late answer, after double thought I think you were right before actually. Checking err should be enough.
If I find a bit of time today, I'll try to replicate to see if its a bug from Node or if your first comments were right

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, looking forward to it!

callback(result);
return;
}
names.forEach(file => {
file = path.join(directory, file);
if (ignore(file)) {
Expand Down