-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Use Node's path helpers under NODERAWFS #17849
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
Changes from 5 commits
f114eec
ed835a0
3137336
07ebf5a
2f43575
2a35049
37b6dd0
9115052
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| /** | ||
| * @license | ||
| * Copyright 2022 The Emscripten Authors | ||
| * SPDX-License-Identifier: MIT | ||
| */ | ||
|
|
||
| mergeInto(LibraryManager.library, { | ||
| $PATH: { | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, it's currently used this way throughout the codebase. Perhaps |
||
| join: function () { | ||
| return nodePath['join'].apply(null, arguments); | ||
| }, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this just be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried that initially, but it caused the acorn-optimizer to fail. |
||
| join2: (l, r) => nodePath['join'](l, r), | ||
| }, | ||
| // The FS-using parts are split out into a separate object, so simple path | ||
| // usage does not require the FS. | ||
| $PATH_FS__deps: ['$FS'], | ||
| $PATH_FS__docs: '/** @type{{resolve: function(...*)}} */', | ||
| $PATH_FS: { | ||
| resolve: function () { | ||
| var paths = Array.prototype.slice.call(arguments, 0); | ||
| paths.unshift(FS.cwd()); | ||
| return nodePath['posix']['resolve'].apply(null, paths); | ||
| }, | ||
| relative: (from, to) => nodePath['posix']['relative'](from || FS.cwd(), to || FS.cwd()), | ||
| } | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment with 37b6dd0. I also removed the "This has mostly been tested on Linux so far"-phrase from
NODERAWFSsettings description with 9115052, since using Node.js operations should be cross-platform (+ AFAIK, CI also test theNODERAWFSsetting on OSes other than Linux).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!