-
Notifications
You must be signed in to change notification settings - Fork 2
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
compatibility patch #13
base: main
Are you sure you want to change the base?
Conversation
this will fix some compatibility issues with some node.js use-cases. This fix will handover the module loading request in node.js to 'require' without breaking the original code in some apps/scripts is that already using this package.
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.
- Please follow the linting standard of the project.
- Please do not use a ternary for this as it is not very easy to read.
- Please include unit tests to cover new and changed code.
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 please add a unit test?
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 please add a unit test?
@@ -1,7 +1,13 @@ | |||
/* eslint-disable no-new-func, camelcase */ | |||
/* globals __non_webpack__require__ */ | |||
|
|||
const realImport = new Function('modulePath', 'return import(modulePath)') | |||
const realImport = typeof process === "undefined" ? new Function('modulePath', 'return import(modulePath)') : async () => { |
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.
This is a commonjs module and this can't be null.
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.
then the check for the process variable should be made in the runtime right?
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 don't understand the question. In Node.js, the process global is always available.
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.
But isn't this module also imported in the browser? I am sorry if I get this module wrong.
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.
This module is not supposed to be used inside browsers.
I also encountered this problem. pino: 8.1.0 This is my source code file
Then I use the pkg@5.8.1 package into an exe program, run the executable program get an error as follows:
I tried several times and found the problem at this location
After using The value of modulePath needs to be processed before using require
PS: This issue is not related to the packaged node version. I tested v14, v16, and v18, all of which reproduced the problem |
This will fix some compatibility issues with some node.js use-cases. This fix will handover the module loading request in node.js to 'require' without breaking the original code in some apps/scripts is that already using this package.
resolves #14