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

feat: add support for win32 paths #402

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

davakh
Copy link

@davakh davakh commented Feb 7, 2024

I implemented general logic for resolving win32 relative paths in library. It's hard to say what else to cover with tests because it's hard to fully mock Node.js's path module and easily switch between win32/posix paths resolving in jest.

If you have thoughts on what else should I cover, I would like to hear about it and I can improve coverage if it's required.

fixes #401

Copy link

linux-foundation-easycla bot commented Feb 7, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Thank you looks good, can we add more test with real examples?

Copy link

codecov bot commented Feb 8, 2024

Codecov Report

Attention: Patch coverage is 61.11111% with 21 lines in your changes missing coverage. Please review.

Project coverage is 92.18%. Comparing base (58464fc) to head (a070b9a).
Report is 64 commits behind head on main.

Files with missing lines Patch % Lines
lib/util/path.js 35.48% 15 Missing and 5 partials ⚠️
lib/SelfReferencePlugin.js 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #402      +/-   ##
==========================================
- Coverage   92.85%   92.18%   -0.67%     
==========================================
  Files          43       44       +1     
  Lines        2042     2085      +43     
  Branches      598      619      +21     
==========================================
+ Hits         1896     1922      +26     
- Misses        118      131      +13     
- Partials       28       32       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alexander-akait
Copy link
Member

Also please fix ts problems, thank you

@alexander-akait
Copy link
Member

@davakh Hello, friendly ping ⭐

@davakh
Copy link
Author

davakh commented Mar 6, 2024

Hello, sorry that it took so long. In new version, I've added tests and rewritten the original path tests based on the fact that this repository has separate runners for Windows. Also, I'm still not sure what you mean by "real examples" and want to ask for a hint about that, but I've added platform-based tests to the main resolve function, so it should cover more cases.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

There are enough tests. Thank you

@davakh
Copy link
Author

davakh commented Mar 7, 2024

I'll take a look on failed tests

@davakh
Copy link
Author

davakh commented Sep 23, 2024

It's hard for me to debug on Windows, and it feels like it requires refactoring of almost all existing tests because they're not based on path.sep from the Node.js path package, and all the existing tests are using POSIX-char separators.

I think it would have a greater impact with a lower cost and a lower chance of breaking something in conclusion if we are going to apply something like this path.relative(....).split(path.win32.sep).join(path.posix.sep); at the beginning of resolving path.

It is hard for me to decide because I don't like this solution. If you agree, I can add this solution to codebase, but if you have any other ideas about solving this, I would like to know about it.

@alexander-akait
Copy link
Member

Yeah, we need refactor our tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Ready for Merge
Development

Successfully merging this pull request may close these issues.

Add ability to handle win32 relative paths
3 participants