-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
std/node: add some Node.js polyfill to require() #3382
Conversation
} | ||
nativeModulePolyfill.set("fs", createNativeModule("fs", nodeFS)); | ||
nativeModulePolyfill.set("util", createNativeModule("util", nodeUtil)); | ||
nativeModulePolyfill.set("path", createNativeModule("path", nodePath)); |
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.
Can you add some documentation of this either to std/node/README.md or the jsdoc for makeRequire ?
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. Also renamed makeRequire
-> createRequire
and require.ts
-> module.ts
. This aligns with https://nodejs.org/dist/latest-v13.x/docs/api/modules.html#modules_module_createrequire_filename better.
@@ -19,6 +19,12 @@ | |||
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE |
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.
I think calling this require.ts
is better. In this context, the name "require", for me, conjures immediately the notion of Node's require; whereas "module" is much more ambiguous and has a more ES-Modules vibe to it (which is not what we want to connote)
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.
Hmm but on the other hand do we want to expose other stuff from this module that are available from Node's module
built-in module?
Since std/node
is intended for Node.js compatibility, IMO it might be better to comply...
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.
ah, right. make sense.
* | ||
* @param filename path or URL to current module | ||
* @return Require function to import CJS modules | ||
*/ |
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.
👍
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.
LGTM
Added
fs
,util
,path
polyfills forrequire
. Future polyfills could be added in the similar way.Also added
global.ts
that setswindow.global = window