doc: update experimental loader hooks example code - #29373
Conversation
Sorry, something went wrong.
|
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 make lint to make sure all the changes fits the general styles. |
Sorry, something went wrong.
|
@devnexen thank you for pointing it out. Commit message is updated and make lint runs without errors now. |
Sorry, something went wrong.
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). |
Sorry, something went wrong.
|
One last approval maybe @lpinca but regardless I may push it soon-ish :-) |
Sorry, something went wrong.
|
Landed in ff78c167b123e |
Sorry, something went wrong.
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/git node land? |
Sorry, something went wrong.
|
Sorry my mistake indeed. |
Sorry, something went wrong.
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! 😀 |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
@devnexen did you not use git node land, or did you not accept the amend option when prompted? I've done the latter before. 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. |
Sorry, something went wrong.
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:
is not cross-platform, it gives wrong URL on windows
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
Checklist