doc: update experimental loader hooks example code #29373
Conversation
|
Hi and thanks for your contribution. Please pay attention to https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#commit-message-guidelines Also |
zaverden
changed the title
|
@devnexen thank you for pointing it out. Commit message is updated and |
It fix 2 issues in provided Loader hooks examples:
1. Original ``new URL(`${process.cwd()}/`, 'file://');``
is not cross-platform, it gives wrong URL on windows
2. Based on `CHECK` in ModuleWrap::Resolve (node 12.9.1,
https://github.com/nodejs/node/blob/v12.9.1/src/module_wrap.cc#L1132)
the 2nd parameter should be a `string`, not an `URL` object
|
I have rebased my branch to the right head (finally). |
|
One last approval maybe @lpinca but regardless I may push it soon-ish :-) |
|
Landed in ff78c167b123e |
Unfortunately, the PR-URL is invalid. It needs to be a full URL. Since this landed 20 hours ago, I think we're stuck with it. Did you do this manually rather than using node-core-utils/ |
|
Sorry my mistake indeed. |
Yeah, it's all right, it's happened before and will happen again. One more reason to get a commit queue going so we don't have to manually land stuff at all! 😀 |
It fix 2 issues in provided Loader hooks examples:
1. Original ``new URL(`${process.cwd()}/`, 'file://');``
is not cross-platform, it gives wrong URL on windows
2. Based on `CHECK` in ModuleWrap::Resolve (node 12.9.1,
https://github.com/nodejs/node/blob/v12.9.1/src/module_wrap.cc#L1132)
the 2nd parameter should be a `string`, not an `URL` object
PR-URL: #29373
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
|
After some brief discussion in #node-dev IRC, I fixed and force-pushed. |
|
@devnexen did you not use We might be able to use github actions to refuse commits that don't have metadata. Personally, I think long-term traceability of the code changes is more important than short-term discomfort of any (possibly none) devs who don't use rebase, and get confused. The main reason to not rewrite history is it can cause pain and confusing branches to get merged... but we won't land confusing branch history, so anyone working on nodejs/node itself won't be affected. |
It fix 2 issues in provided Loader hooks examples:
1. Original ``new URL(`${process.cwd()}/`, 'file://');``
is not cross-platform, it gives wrong URL on windows
2. Based on `CHECK` in ModuleWrap::Resolve (node 12.9.1,
https://github.com/nodejs/node/blob/v12.9.1/src/module_wrap.cc#L1132)
the 2nd parameter should be a `string`, not an `URL` object
PR-URL: #29373
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
It fix 2 issues in provided Loader hooks examples:
1. Original ``new URL(`${process.cwd()}/`, 'file://');``
is not cross-platform, it gives wrong URL on windows
2. Based on `CHECK` in ModuleWrap::Resolve (node 12.9.1,
https://github.com/nodejs/node/blob/v12.9.1/src/module_wrap.cc#L1132)
the 2nd parameter should be a `string`, not an `URL` object
PR-URL: #29373
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
It fix 2 issues in provided Loader hooks examples:
new URL(`${process.cwd()}/`, 'file://');is not cross-platform, it gives wrong URL on windows
CHECKin ModuleWrap::Resolve (node 12.9.1,https://github.com/nodejs/node/blob/v12.9.1/src/module_wrap.cc#L1132)
the 2nd parameter should be a
string, not anURLobjectChecklist