Skip to content

fix: Normalize paths and line endings for cross-platform compatibility#631

Closed
marcogomez-dev wants to merge 10 commits intonuejs:masterfrom
marcogomez-dev:fix-windows-normalizations-paths-and-trailingslash
Closed

fix: Normalize paths and line endings for cross-platform compatibility#631
marcogomez-dev wants to merge 10 commits intonuejs:masterfrom
marcogomez-dev:fix-windows-normalizations-paths-and-trailingslash

Conversation

@marcogomez-dev
Copy link

@marcogomez-dev marcogomez-dev commented Oct 15, 2025

fix #621
Fix #630
Fix #629
Fix #628

  • Replaced platform-specific path separators with POSIX format (/) in deps.js, file.js, serve.js, and fswalk.js
  • Updated line ending processing in compiler.js to remove carriage returns (\r)
  • Added directory redirection to trailing slash in serve.js when index files exist

Changes ensure consistent file path handling and text processing across Windows, Linux, and macOS platforms.

@nobkd
Copy link
Collaborator

nobkd commented Oct 15, 2025

Hey, thanks for the pr.

Some comments :)

For the unzip issue, there's already an open pr (See #622). Your changes also don't work on all OSes, als tar can't unzip on every device. Windows tar supports zip, but Unix systems have the unzip command, that specifically is for zips compared to the tar command (which may or may not support extracting zip). So, maybe just revert that part.

You left some non English comments in the code.

The "fix #...." keywords need to be mentioned in the top pr description, to be automatically applied when the pr is merged (you can see in the sidebar, which PRs are referenced for closing)


I plan to take a deeper look at this pr around tomorrow evening.
Nothing you have to worry about right now, but I think it would've been better to make separate PRs for the different change topics: like one for normalizing path separators, one for line endings, (though you could count both of them as part of windows handling), and espacially the missing trailing / redirection thingy I would've extracted to a separate pr. But this is just my personal opinion on this matter, and a consideration you can have in mind for future contributions. Thank you.

@marcogomez-dev
Copy link
Author

Hey, thanks for the pr.

Some comments :)

For the unzip issue, there's already an open pr (See #622). Your changes also don't work on all OSes, als tar can't unzip on every device. Windows tar supports zip, but Unix systems have the unzip command, that specifically is for zips compared to the tar command (which may or may not support extracting zip). So, maybe just revert that part.

You left some non English comments in the code.

The "fix #...." keywords need to be mentioned in the top pr description, to be automatically applied when the pr is merged (you can see in the sidebar, which PRs are referenced for closing)

I plan to take a deeper look at this pr around tomorrow evening. Nothing you have to worry about right now, but I think it would've been better to make separate PRs for the different change topics: like one for normalizing path separators, one for line endings, (though you could count both of them as part of windows handling), and espacially the missing trailing / redirection thingy I would've extracted to a separate pr. But this is just my personal opinion on this matter, and a consideration you can have in mind for future contributions. Thank you.

Hello there!

Thanks so much for taking the time to review my PR and for all the insightful comments! I really appreciate the guidance.

I apologize for the confusion regarding the tar command. I did some research and found documentation suggesting it supported the ZIP format, but after digging deeper, it seems that on Windows and macOS, they often use BSDTAR, which is compatible with ZIP, whereas many Unix systems use GNU TAR, which is not. That was a great catch, and I'll definitely revert that part.

Regarding the non-English comments, you are absolutely right—I noticed that late! I've already made a note to ensure all comments are in English moving forward.

Also, I completely agree that separating the changes into different PRs (one for path/line endings, one for the trailing slash redirect) would have been much cleaner. It's a great piece of advice, and I welcome all your feedback since I'm quite new to this. Honestly, just getting the first PR opened felt like a major accomplishment! 😄

I'm going to follow your advice: I will prepare another commit now, switching the comments to English and reverting the change on tar.

For this specific PR, I'll keep the changes bundled together, as I'm a bit unsure how to best separate them now without starting over. But I will definitely keep the "separate PRs for separate concerns" advice in mind for all my future contributions.

Thank you again for all the support, help, and guidance! It means a lot!

@appurist
Copy link

@marcogomez-dev For what it's worth, I just pulled this PR on my Windows machine and I can confirm the full template is functioning as it should (as best as I can tell, whereas it had many problems before these fixes. (I haven't tried Mac/Linux.)

Copy link
Collaborator

@nobkd nobkd left a comment

Choose a reason for hiding this comment

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

first look

@nobkd nobkd requested a review from tipiirai October 16, 2025 23:27
@marcogomez-dev
Copy link
Author

@nobkd Hello! Thanks for taking your time reviewing my code, and thanks for your suggestions and guidance. I saw your comments and I've made all the changes. Can you tell me if it's all okay now?

@nobkd
Copy link
Collaborator

nobkd commented Oct 17, 2025

normalize does not convert from \\ to /, it is for resolving . and .. parts in a path, where possible. I'm not sure you used it with that intention everywhere.

The path.normalize() method normalizes the given path, resolving '..' and '.' segments.

I'll take a more thorough look in a few

@nobkd nobkd force-pushed the fix-windows-normalizations-paths-and-trailingslash branch from fb60357 to c2b252c Compare October 21, 2025 18:05
Marco Gómez added 10 commits October 21, 2025 20:06
- Replaced platform-specific path separators with POSIX format (/) in deps.js, file.js, serve.js, and fswalk.js
- Updated line ending processing in compiler.js to remove carriage returns (\r)
- Added directory redirection to trailing slash in serve.js when index files exist

Changes ensure consistent file path handling and text processing across Windows, Linux, and macOS platforms.
- Disable source maps in JS compilation to fix failure
- Refactor unzip function to use tar for multi-platform extraction with proper cleanup
- Normalize paths to POSIX and improve build filtering for shared data dirs
- Update imports in create.js for new file operations in unzip logic
…s BSD TAR and GNU TAR, for windows, macOs and unix systems)
@nobkd nobkd force-pushed the fix-windows-normalizations-paths-and-trailingslash branch from c2b252c to 05a1d4c Compare October 21, 2025 18:12
@nobkd
Copy link
Collaborator

nobkd commented Oct 21, 2025

rebased on master


error: expect(received).toContain(expected): packages\nuekit\test\fswatch.test.js#L74
Expected to contain: "newdir/file1.txt"
Received: [ "newdir\\file1.txt", "newdir\\file2.js", "newdir\\file1.txt", "newdir\\file2.js",
  "newdir\\file1.txt", "newdir\\file2.js"
]

      at <anonymous> (D:\a\nue\nue\packages\nuekit\test\fswatch.test.js:74:19)

As I said, normalize does sadly not convert from \\ to /. (I'm not so sure anymore, I'll do some tests around that later)

I wish there was a builtin function. If anyone knows about a builtin function to convert to posix path, that would be nice.

I still think it would be alright, to convert to posix in all places, where the path first comes into contact with nue, e.g. in both fswalk and fswatch, and just only use the posix variant of path everywhere else: like import {...} from 'node:path/posix'.

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

Labels

None yet

Projects

None yet

3 participants