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

Windows path issues #18

Closed
Dids opened this issue Dec 20, 2015 · 5 comments
Closed

Windows path issues #18

Dids opened this issue Dec 20, 2015 · 5 comments

Comments

@Dids
Copy link

Dids commented Dec 20, 2015

I'm using path.normalize() basically everywhere, but yazl is throwing me the following error:

Error: invalid characters in path: fonts\fontawesome-webfont.eot

Is yazl cross-platform/compatible with Windows?

@Dids
Copy link
Author

Dids commented Dec 20, 2015

I've created pull request #19 that fixes this (tested on Windows only, but I don't see why it wouldn't also work on Linux and OS X).

@thejoshwolfe
Copy link
Owner

Yes, yazl is compatible with Windows, but the zip file format does not allow native Windows paths, as I'm sure you've discovered. This is documented in yazl's API "A valid metadataPath...".

#19 adds a convenience layer of translation for the metadataPath similar to what we're already doing for appending a trailing / for directory entries. it converts illegal \ characters into legal / characters. This translation seems pretty safe. I like it.

However, #19 also calls path.normalize() which doesn't seem right. If path.normalize() changes something, that seems like there really was a problem with the input. path.normalize() is only required for certain types of applications that build paths using complex logic or user input. In these cases, it seems like it's the calller's responsibility to normalize the path before passing it in as an argument. Furthermore, yazl throwing an exception on non-normalized input might be helpful to catch some kinds of errors.

In general, I don't want yazl's API to be too high-level, but translating Windows paths to Unix paths seems ok.

I'll take some action on this when i get home later today.

@Dids
Copy link
Author

Dids commented Dec 21, 2015

I've previously used path.* helpers to abstract away any platform specific tasks, including normalizing the path for any OS that might be running the app. That said, I agree that it shouldn't be necessary here, and I only included it due to old habits dying hard.

Simply doing the slash conversion should work just fine. I'll be happy with any solution, as long as this gets addressed at some point. :)

@thejoshwolfe
Copy link
Owner

I'll make a release when I get #6 implemented as well.

@thejoshwolfe
Copy link
Owner

I'll make a release when I get #6 implemented as well.

Never mind. 2.3.0 is released now with this change.

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

No branches or pull requests

2 participants