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: keep internal consistency with glob and paths #303

Merged
merged 9 commits into from
Aug 16, 2024

Conversation

wkillerud
Copy link
Contributor

Windows paths threw a spanner in the works. This might not be the
optimal solution for outside modules, in which case I'm open to
changing the behavior to maintain win32 style paths on that OS.
The reasoning here is to keep in line with glob (which recommends
only using forward slashes in patterns) and the URLs which end up
on the Eik server (which, being HTTP URLs, use forward slash).

This should hopefully fix an issue Windows users have with the Eik
CLI not finding the files configured in eik.json

@wkillerud wkillerud linked an issue Jul 5, 2024 that may be closed by this pull request
Copy link
Contributor Author

@wkillerud wkillerud left a comment

Choose a reason for hiding this comment

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

It's getting late on a Friday. I'll pick this up later.

test/classes/local-file-location/absolute.test.js Outdated Show resolved Hide resolved
test/classes/eik-config/mappings.test.js Outdated Show resolved Hide resolved
test/classes/eik-config/mappings.test.js Outdated Show resolved Hide resolved
lib/helpers/resolve-files.js Show resolved Hide resolved
lib/helpers/resolve-files.js Outdated Show resolved Hide resolved
@trygve-lie
Copy link
Contributor

Is the problem here that we get wrong separator on Windows which does not match with "URL" path separators? Not sure I 100% understand the issue.

Though; Glob support returning a POSIX separator independent of the underlying OS by setting posix as an config.

posix Set to true to use / as the path separator in returned results. On posix systems, this has no effect. On Windows systems, this will return / delimited path results, and absolute paths will be returned in their full resolved UNC path form, eg insted of 'C:\foo\bar', it will return //?/C:/foo/bar.

Would setting this on the glob avoid all this separator checking / converting in different places in the code?

@wkillerud
Copy link
Contributor Author

There are a few issues this PR tries to solve:

  • The first is that we get different results from Node's path functions on Windows, and we use those to form the input to glob. Here we ensure the input to glob uses forward slashes regardless of OS.
  • Another issue is that we use join to form the remote file location (in eik-config.js), which should be with forward slashes.
  • A third issue is that the paths in the input (say from eik.json) used as input to Node's path functions don't necessarily match the OS's separators. If config/is/like/this it could perhaps cause issues on Windows 🤔

@wkillerud
Copy link
Contributor Author

This issue is starting to back up the pull request queues now that they run on Windows hosts. We could skip Windows for now, but it would be nice to get this merged in one state or another.

wkillerud and others added 7 commits August 15, 2024 13:19
Windows paths threw a spanner in the works. This might not be the
optimal solution for outside modules, in which case I'm open to
changing the behavior to maintain win32 style paths on that OS.
The reasoning here is to keep in line with glob (which recommends
only using forward slashes in patterns) and the URLs which end up
on the Eik server (which, being HTTP URLs, use forward slash).

This should hopefully fix an issue Windows users have with the Eik
CLI not finding the files configured in eik.json
Not active unless passing --only, but still
@wkillerud wkillerud merged commit 4e6a307 into main Aug 16, 2024
6 checks passed
@wkillerud wkillerud deleted the windows-resolve-files branch August 16, 2024 07:09
github-actions bot pushed a commit that referenced this pull request Aug 16, 2024
## [4.1.1](v4.1.0...v4.1.1) (2024-08-16)

### Bug Fixes

* windows support ([#303](#303)) ([4e6a307](4e6a307))
Copy link

🎉 This issue has been resolved in version 4.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Eik-cli doesn't work on Windows
3 participants