Skip to content
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

doc: document resolve hook formats #16375

Closed
wants to merge 1 commit into from
Closed

doc: document resolve hook formats #16375

wants to merge 1 commit into from

Conversation

azz
Copy link
Contributor

@azz azz commented Oct 22, 2017

Noticed format: "dynamic" was missing from the docs (it's mentioned later on).

Add "dynamic" to the list of supported formats returned by a custom resolve hook.

Add a table describing the meaning of each format.

Refs: #15445

Checklist
Affected core subsystem(s)

doc

cc. @guybedford, @bmeck

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Oct 22, 2017
doc/api/esm.md Outdated
| `"cjs"` | Load a node-style CommonJS module |
| `"builtin"` | Load a node builtin CommonJS module |
| `"json"` | Load a JSON file |
| `"addon"` | Load a [C++ Addon](./addons.md) |
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Oct 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this should be (addons.html) for referring to the compiled doc,

doc/api/esm.md Outdated
| `"builtin"` | Load a node builtin CommonJS module |
| `"json"` | Load a JSON file |
| `"addon"` | Load a [C++ Addon](./addons.md) |
| `"dynamic"` | Use a [dynamic instantiate hook](#dynamic-instantiate-hook) |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same: in the compiled doc the hash is (#esm_dynamic_instantiate_hook)

@vsemozhetbyt
Copy link
Contributor

Hi @azz! Thank you for improving the docs! You can add a fixing commit to this branch or amend the existing one. Let us know if you need any help with this.

@azz
Copy link
Contributor Author

azz commented Oct 22, 2017

Fixed. Thanks for catching that!

Add `"dynamic"` to the list of supported `format`s returned by a
custom resolve hook.

Add a table describing the meaning of each `format`.

Refs: nodejs#15445
@azz
Copy link
Contributor Author

azz commented Oct 22, 2017

Changed it to follow the convention of using link references.

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Format part LGTM

@hiroppy hiroppy added the esm Issues and PRs related to the ECMAScript Modules implementation. label Nov 10, 2017
@BridgeAR
Copy link
Member

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 23, 2017
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

landing...

@targos
Copy link
Member

targos commented Nov 23, 2017

Landed in e5d4aeb

@targos targos closed this Nov 23, 2017
targos pushed a commit that referenced this pull request Nov 23, 2017
Add `"dynamic"` to the list of supported `format`s returned by a
custom resolve hook.

Add a table describing the meaning of each `format`.

PR-URL: #16375
Refs: #15445
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 28, 2017
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
Add `"dynamic"` to the list of supported `format`s returned by a
custom resolve hook.

Add a table describing the meaning of each `format`.

PR-URL: #16375
Refs: #15445
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
Add `"dynamic"` to the list of supported `format`s returned by a
custom resolve hook.

Add a table describing the meaning of each `format`.

PR-URL: #16375
Refs: #15445
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
Add `"dynamic"` to the list of supported `format`s returned by a
custom resolve hook.

Add a table describing the meaning of each `format`.

PR-URL: #16375
Refs: #15445
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
Add `"dynamic"` to the list of supported `format`s returned by a
custom resolve hook.

Add a table describing the meaning of each `format`.

PR-URL: #16375
Refs: #15445
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants