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

Core: Fix staticDirs path issue on Windows #17641

Merged
merged 1 commit into from
Mar 9, 2022

Conversation

justrhysism
Copy link
Contributor

@justrhysism justrhysism commented Mar 7, 2022

Issue: #17271

What I did

Noticed that the test "supports absolute file paths with custom endpoint" didn't cover the reality for Windows.

The relativeDir provided by getDirectoryFromWorkingDir() resolves all paths to OS separators (e.g. \ on Windows). The result of which would look like C:\\foo\\bar:\\custom-endpoint and not C:\\foo\\bar:/custom-endpoint which the test was covering; so I added a test to cover the actual case.


Digging in, the regex split in parseStaticDir didn't account for the actual path, as it was ignoring all cases of :\ to cover the X:\ scenario, which is what resulted in bug #17271.

To work around this, I changed the code to look for the last colon :, and use that to split. However, in case the last colon is actually part of a Windows absolute path (e.g. C:\), needed to check a few conditions:

  • Is the path Windows absolute? (We don't care if Unix absolute because it won't have a colon)
  • Is the path just a single path (i.e. doesn't contain the : delimiter for static and target dirs.

Once this was determined, the final piece of the puzzle was ensuring the target was converted from \ to /, for which I just split on path.sep and joined on path.posix.sep to ensure a forward-slash /.


How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented Mar 7, 2022

☁️ Nx Cloud Report

CI ran the following commands for commit 6f9c8a5. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@justrhysism
Copy link
Contributor Author

The failing e2e test doesn't appear to have anything to do with the code I've changed?

@justrhysism justrhysism changed the title Resolve #17271 staticDirs path issue on Windows fix: #17271 staticDirs path issue on Windows Mar 7, 2022
@justrhysism justrhysism changed the title fix: #17271 staticDirs path issue on Windows Fix staticDirs path issue on Windows Mar 7, 2022
@shilman
Copy link
Member

shilman commented Mar 7, 2022

@tooppaaa can you please kick the tires on windows? 🙏

@tooppaaa
Copy link
Contributor

tooppaaa commented Mar 7, 2022

Everything looks good on windows ! 🚀

@@ -72,6 +72,13 @@ describe('parseStaticDir', () => {
targetDir: './custom-endpoint',
targetEndpoint: '/custom-endpoint',
});

await expect(parseStaticDir('C:\\foo\\bar:\\custom-endpoint')).resolves.toEqual({
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
await expect(parseStaticDir('C:\\foo\\bar:\\custom-endpoint')).resolves.toEqual({
await expect(parseStaticDir('C:\\foo\\bar:/custom-endpoint')).resolves.toEqual({

shouldn't this be a forward slash since it's a URL?

Copy link
Contributor Author

@justrhysism justrhysism Mar 8, 2022

Choose a reason for hiding this comment

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

So, the underlying issue is that getDirectoryFromWorkingDir() in useStatics returns this "URL" with backslashes; see arg in the screencap:

image

I'm sure that parseStaticDir is intended to receive the path as a URL, but the reality is that it might not.

As such, I've left the original test with /custom-endpoint and added \\custom-endpoint to ensure this case is covered.


I did briefly consider fixing the underlying issue inside useStatics, however that seemed like a much riskier fix with a much, much broader footprint. To me the safer course of action was to make parseStaticDir more forgiving (not to mention significantly easier for my first contrib).

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the clarification! 🙏

@shilman shilman changed the title Fix staticDirs path issue on Windows Core: Fix staticDirs path issue on Windows Mar 9, 2022
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Thanks so much for the great fix @justrhysism !!! 🙏

@IanVS
Copy link
Member

IanVS commented Mar 9, 2022

BTW, to run unit tests on windows, you can add the ci:matrix label to the PR. Just in case that's helpful for anyone in the future. Maybe we should trigger on windows too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants