-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Don't assume paths are always valid UTF8 #627
Comments
For APIs that take a string for a path wouldn't changing the messages to take byte vectors be redundant as aren't JavaScript strings are guaranteed to be UTF-8, and then to use them as |
Not really actually, technically javascript uses a fixed length 16-bit encoding known as UCS2 (just like java and windows actually). Usually we (browsers, developers) pretend that it uses UTF16, otherwise there'd be no way to encode smileys. But try this in devtools:
Conversions between
So does javascript :)
I'm sure we can break into it somehow.... |
I'm working on this as part of what started as refactoring the is_remote function. We are given UTF-8 encoded strings as arguments so a conversion is inevitable. But from the moment the module resolution starts, they are converted to Rust Paths or Hyper URIs. This won't win the war, but it's an important battle. |
@piscisaureus Thank you for explaining. I don't think that would solve the problem presented in the Node issue as you can't encode a string literal as I think the best solution would be to have a facsimile to Rust's
class OsString {
private buffer: UInt8Array | UInt16Array;
constructor(buffer: UInt8Array | UInt16Array) {
if (windows && buffer instanceof UInt8Array)) {
throw new Error("OsString requires UInt16Array on Windows");
} else if (unix && buffer instanceof UInt16Array)) {
throw new Error("OsString requires UInt8Array on Unix platforms");
}
this.buffer = buffer;
}
/* @internal */
into_msg(builder: fbs.Builder): fbs.OsString {
// Convert and return as flatbuffer message
}
} |
You can, if you hold it in an ArrayBuffer and do the conversion with TextEncoder/TextDecoder. Holding it in a Uint8Array (low level JavaScript API) is what is being suggested here if I understand correctly. |
@benjamingr Sorry that was explicitly about first two points and for keeping to use Strings/strs for that part of the API and that changing those parts would only add redundancy. Sorry if that was unclear.
|
@Aaronepower I already started working on using Paths/URIs instead of Paths on the Rust side. I'm not sure about using byte vectors to encode paths instead of strings. We are going to have to convert paths between textual and canonical representation, this is just shuffling it around to JS's side. |
@NewLunarFire You can assume that import paths are strings and unicode encoded (if not ascii) - we will not be supporting importing arbitrarily encoded filenames. This issue is about supporting file system ops for non-utf8 filenames (a more general problem). |
Does that mean that String.prototype.normalize() is not going to work as expected? |
`deno` only accepts absolute paths with file extension specified. Turned out that tests failed because of an issue in `deno`. See [denoland/deno#627] to learn more.
I think the solution to this problem will depend on the following questions:
I'd say the most reasonable answers are "yes" and "no", respectively. I propose the following approach:
What do you think? |
I'm not convinced it's useful to support non-unicode filenames. It's a huge complexity for what is a legacy problem at this point. It's very nice to assume paths are strings. Related: in #4004 we skip non-unicode filenames. I think this is reasonable. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This is a longer term goal.
This was an issue in Node: nodejs/node-v0.x-archive#2387
The text was updated successfully, but these errors were encountered: