Use Node's path helpers under NODERAWFS #17849
Conversation
| isAbs: (path) => nodePath['isAbsolute'](path), | ||
| normalize: (path) => nodePath['normalize'](path), | ||
| dirname: (path) => nodePath['dirname'](path), | ||
| basename: (path) => nodePath['basename'](path), |
There was a problem hiding this comment.
I'm curious why closure wouldn't know how to deal with nodePath without the extra quoting here?
There was a problem hiding this comment.
Good point, it's currently used this way throughout the codebase. Perhaps nodePath should be placed within src/closure-externs/closure-externs.js?
| basename: (path) => nodePath['basename'](path), | ||
| join: function () { | ||
| return nodePath['join'].apply(null, arguments); | ||
| }, |
There was a problem hiding this comment.
Can this just be join: nodePath['join']? (same with above)
There was a problem hiding this comment.
I tried that initially, but it caused the acorn-optimizer to fail.
|
Tested it with the scenarios I had in #17821- works great. Thanks. I had added some window specific tests in that PR, @kleisauke do you think they would be appropriate here? |
|
@rchiodo Great, thanks for testing! I just added a simple test that previously failed on Windows. |
Ensures that absolute paths are handled correctly on Windows as well. Using these helpers should be safe, since binaries linked with -sNODERAWFS can only be used on Node.js.
It already resolves to `FS.cwd()` by default, so there's no need to pass it as first argument.
The `PATH_FS.resolve` invocation above already made the path absolute.
kleisauke
force-pushed
the
issue-17360
branch
from
a5dc19c to
07ebf5a
Compare
|
Rebased now that PR #17851 has been merged. |
|
Is there anything I can do to make this land? AFAIK, |
| * Copyright 2022 The Emscripten Authors | ||
| * SPDX-License-Identifier: MIT | ||
| */ | ||
|
|
There was a problem hiding this comment.
Maybe add a comment in this file explaining the motivation for it? (which IIUC from the discussion is windows?) lgtm with that.
There was a problem hiding this comment.
Ensures that absolute paths are handled correctly on Windows as well. Using these helpers should be safe, since binaries linked with -sNODERAWFS can only be used on Node.js.
Ensures that absolute paths are handled correctly on Windows as well. Using these helpers should be safe, since binaries linked with
-sNODERAWFScan only be used on Node.js, see:emscripten/src/library_noderawfs.js
Line 28 in e98554f
Context: #16296 (comment).
Resolves: #17360.
Resolves: #17819.
Supersedes: #17821.
(If preferred, I can split the last
32 non-functional changes)