-
-
Notifications
You must be signed in to change notification settings - Fork 38
fix: run tests on windows #127
fix: run tests on windows #127
Conversation
Not sure why GH actions hasn't run CI for this, but hey look, it ran in my fork: https://github.com/achingbrain/dependency-check/actions/runs/362928866 |
There are two things that break this module on windows: 1. Quoting globbed strings to stop unix shells doing path expansion results in the quotes being passed through to globby which breaks 2. Normalising the paths replaces backlashes with forward slashes on Windows. Because the code passes them to globby twice, the second time round nothing is matched because it doesn't understand forward slashes as path separators So in this PR 1. Strip any leading and trailing quotes from strings 2. Do the path.resolve dance to dedupe globby's output but return the original strings instead of the resolved strings
a44c0ea
to
81c9e95
Compare
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.
Thanks for this! Awesome that you took time to get this set up correctly for windows 🙏
Added some helpful comments, I hope you don't mind, not meant to criticize you in any way.
index.js
Outdated
const paths = Object.values(resolvedEntries.reduce((result, entry) => { | ||
// Globby yields unix-style paths. | ||
const normalized = path.resolve(entry) | ||
|
||
if (!result[normalized]) { | ||
result[normalized] = true | ||
result[normalized] = entry | ||
} | ||
|
||
return result |
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 makes the normalized
variable rather redundant, no? So we could just skip this entire reduce
and just return resolvedEntries
straight up?
Makes me wonder why its originally been added there at all 🤔
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.
As I read it, the reason you resolve the path is to make all the requires absolute and then use the result
hash to dedupe them?
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.
That's true, good catch.
Because the code passes them to globby twice, the second time round nothing is matched because it doesn't understand forward slashes as path separators
I'm thinking that we maybe instead should ensure that whatever globby is given is always using the right slashes? Adding a entry.replace('\', '/')
to
Lines 26 to 35 in ad715e5
async function resolveGlobbedPath (entries, cwd) { | |
if (typeof entries === 'string') entries = [entries] | |
debug('globby resolving', entries) | |
const resolvedEntries = await globby(entries, { | |
cwd, | |
absolute: true, | |
expandDirectories: false | |
}) |
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.
It seems to work with replacing the slashes before passing the entries to globby, I've updated the PR.
Co-authored-by: Pelle Wessman <pelle@kodfabrik.se>
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.
Splendid!
There are two things that break this module on windows:
results in the quotes being passed through to globby which breaks
Windows. Because the code passes them to globby twice, the second
time round nothing is matched because it doesn't understand forward
slashes as path separators
So in this PR
original strings instead