Skip to content

doc: update experimental loader hooks example code - #29373

Closed
zaverden wants to merge 1 commit into
nodejs:masterfrom
zaverden:patch-1
Closed

doc: update experimental loader hooks example code#29373
zaverden wants to merge 1 commit into
nodejs:masterfrom
zaverden:patch-1

Conversation

@zaverden

@zaverdenzaverden commented Aug 30, 2019

Copy link
Copy Markdown
Contributor

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
Checklist

@nodejs-github-botnodejs-github-bot added the doc Issues and PRs related to the documentations. label Aug 30, 2019
@devnexen

Copy link
Copy Markdown
Contributor

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.

@zaverdenzaverden changed the title Update Experimental Loader hooks example codedoc: update experimental loader hooks example codeAug 30, 2019
@zaverden

Copy link
Copy Markdown
ContributorAuthor

@devnexen thank you for pointing it out. Commit message is updated and make lint runs without errors now.

Comment threaddoc/api/esm.md Outdated
Comment threaddoc/api/esm.md Outdated
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
@zaverden

Copy link
Copy Markdown
ContributorAuthor

I have rebased my branch to the right head (finally).
@lpinca@vsemozhetbyt do you have any other comments?

@devnexen

Copy link
Copy Markdown
Contributor

One last approval maybe @lpinca but regardless I may push it soon-ish :-)

@devnexen

Copy link
Copy Markdown
Contributor

Landed in ff78c167b123e

@zaverden
zaverden deleted the patch-1 branch September 17, 2019 04:20
@Trott

Copy link
Copy Markdown
Member

Landed in ff78c16

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?

@devnexen

Copy link
Copy Markdown
Contributor

Sorry my mistake indeed.

@Trott

Copy link
Copy Markdown
Member

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! 😀

Trott pushed a commit that referenced this pull request Sep 17, 2019
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>
@Trott

Copy link
Copy Markdown
Member

After some brief discussion in #node-dev IRC, I fixed and force-pushed.

@sam-github

Copy link
Copy Markdown
Contributor

@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.

targos pushed a commit that referenced this pull request Sep 20, 2019
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>
@BridgeARBridgeAR mentioned this pull request Sep 24, 2019
BridgeAR pushed a commit that referenced this pull request Sep 25, 2019
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>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docIssues and PRs related to the documentations.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@zaverden@devnexen@Trott@sam-github@jasnell@lpinca@vsemozhetbyt@nodejs-github-bot