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

Always use forward slash on import paths #346

Merged
merged 4 commits into from
Aug 9, 2024

Conversation

gustavo-shigueo
Copy link
Collaborator

@gustavo-shigueo gustavo-shigueo commented Aug 7, 2024

Goal

This PR changes import paths such that they will always use forward slash / as the path separator, even on Windows. It also adds a windows test run to the CI
Closes #345

Changes

If target_os = "windows", replace \ with /

Checklist

  • I have followed the steps listed in the Contributing guide.
  • If necessary, I have added documentation related to the changes made.
  • I have added or updated the tests related to the changes made.

_ => rel_path.to_string_lossy().into(),
};

let path = if cfg!(target_os = "windows") {
str_path.replace('\\', "/")
Copy link
Collaborator

@NyxCode NyxCode Aug 7, 2024

Choose a reason for hiding this comment

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

I dont think this is sufficient, since paths can contain \.

We might need to iterate over the path components, not sure though.

Copy link
Collaborator Author

@gustavo-shigueo gustavo-shigueo Aug 7, 2024

Choose a reason for hiding this comment

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

I dont think this is sufficient, since paths can contain \.

Wdym? Is a component allowed to contain \? This replaces every \ for /

Copy link
Collaborator Author

@gustavo-shigueo gustavo-shigueo Aug 8, 2024

Choose a reason for hiding this comment

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

It seems \ is allowed in Unix file/directory names, but this shouldn't be a problem, as the replace only happens if the user's OS is Windows, where \ is not allowed in file/directory names.

I also suspect TS itself would not be very fond of a file containaing \ in its name.

It could even be a reasonable idea to throw a compile time error if #[ts(export_to)] or TS_RS_EXPORT_DIR contain backslashes as that would probably produce unintended behavior and destroy portability, by having a path that can't exist on Windows. Of course, doing so would be another of those "technically a breaking breaking change, but probably won't affect anyone" versions

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you're right, sorry about that!

I thought I remembered some weird rules for paths on windows, but i was mistaken.

Copy link
Collaborator

@NyxCode NyxCode left a comment

Choose a reason for hiding this comment

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

thanks man! especially happy about the windows CI!

@gustavo-shigueo gustavo-shigueo merged commit 78591a2 into main Aug 9, 2024
18 checks passed
@gustavo-shigueo gustavo-shigueo deleted the always_use_forward_slash branch August 9, 2024 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dependency paths in Windows use back slash, though they should use the canonical forward slash
2 participants