windows pathing fix #53
Conversation
windows pathing fix
| @@ -18,6 +18,14 @@ const data = Buffer.from(dataBuf) | |||
| async function start () { | |||
| let fn | |||
| try { | |||
| if(typeof process !== "undefined"){ | |||
There was a problem hiding this comment.
Please apply the linting standard of the project. npm test will report an error here.
| @@ -18,6 +18,14 @@ const data = Buffer.from(dataBuf) | |||
| async function start () { | |||
| let fn | |||
| try { | |||
| if(typeof process !== "undefined"){ | |||
| //reused the error code. | |||
There was a problem hiding this comment.
What is this comment referencing?
| @@ -18,6 +18,14 @@ const data = Buffer.from(dataBuf) | |||
| async function start () { | |||
| let fn | |||
| try { | |||
| if(typeof process !== "undefined"){ | |||
| //reused the error code. | |||
| const err = new Error() | |||
There was a problem hiding this comment.
| const err = new Error() | |
| const err = Error() |
Also, an appropriate message should be passed to the constructor.
| if(typeof process !== "undefined"){ | ||
| //reused the error code. | ||
| const err = new Error() | ||
| err.code = 'ERR_MODULE_NOT_FOUND'; |
There was a problem hiding this comment.
This code does not seem appropriate for the condition.
| //this will not universally work on all node.js instances and use-cases. | ||
| //better to hand it over to require when using node.js |
There was a problem hiding this comment.
I don't understand this comment.
There was a problem hiding this comment.
This is for pkg incompatibility.
| @@ -30,7 +38,7 @@ async function start () { | |||
| // The error codes may change based on the node.js version (ENOTDIR > 12, ERR_MODULE_NOT_FOUND <= 12 ) | |||
| if ((error.code === 'ENOTDIR' || error.code === 'ERR_MODULE_NOT_FOUND') && | |||
| workerData.filename.startsWith('file://')) { | |||
| fn = realRequire(workerData.filename.replace('file://', '')) | |||
| fn = realRequire(workerData.filename.replace(workerData.filename[9] === ":" ? 'file:///' : 'file://', '')) | |||
There was a problem hiding this comment.
This seems very fragile. I think the new code is looking for file://c:/foo/bar. But there is no guarantee the provided path will be absolute. I think a proper process.platform check should be used instead.
There was a problem hiding this comment.
I agree this is fragile. I agree to use process.platform
The old code removes the file:// from the path. in unix systems. file:///example/ex.js will be : /example/ex.js. But in windows file:///C:/example/ex.js will be : /C:/example.js and this will make the module loading fail to resolve the file.
|
@simoneb could you check this on Windows? |
happy to check once there's a proper implementation and tests in this PR |
import will not universally work on all node.js use-cases like pkg for example. I made fix to handover the module loading to require in node.js for better compatibility.
when removing the 'file://' from path, in windows, it will leave a leading '/' in the path that will make the worker module loading fail.