-
Notifications
You must be signed in to change notification settings - Fork 896
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 fix #1254
compatibility fix #1254
Conversation
This will fix some incompatibility issues with some node.js use-cases by handling over the module loading to require. This patch is dependent also to the patches I made with the thread-stream and real-require.
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 provide unit test to cover new and changed code.
@@ -13,6 +13,12 @@ module.exports = async function ({ targets }) { | |||
targets = await Promise.all(targets.map(async (t) => { | |||
let fn | |||
try { | |||
if(typeof process !== "undefined"){ |
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 rules of this project.
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.
How can this condition happen?
@@ -13,6 +13,12 @@ module.exports = async function ({ targets }) { | |||
targets = await Promise.all(targets.map(async (t) => { | |||
let fn | |||
try { | |||
if(typeof process !== "undefined"){ | |||
//reuse the error 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.
What is this comment referencing?
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 am sorry. I meant that I reused the error code for the condition to match in the if statement catch clause.
@@ -13,6 +13,12 @@ module.exports = async function ({ targets }) { | |||
targets = await Promise.all(targets.map(async (t) => { | |||
let fn | |||
try { | |||
if(typeof process !== "undefined"){ | |||
//reuse the error code. | |||
const err = new Error() |
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.
const err = new Error() | |
const err = Error() |
Also, an appropriate message should be passed to the constructor.
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.
We should not emit Node core error codes
if(typeof process !== "undefined"){ | ||
//reuse the error code. | ||
const err = new Error() | ||
err.code = 'ERR_MODULE_NOT_FOUND'; |
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.
Is this code appropriate for the condition?
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.
Tests are needed
I was long away from from the project where I used this. I remembered it as someone proposed a better fix. I tried it and it was working. I should close this now. |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This will fix some incompatibility issues with some node.js use-cases by handling over the module loading to require. This patch is dependent also to the patches I made with the thread-stream and real-require.
resolves #1237