FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: fix ESM hook mistypes and links to types by DerekNonGeneric · Pull Request #34240 · nodejs/node · GitHub

/ node Public

doc: fix ESM hook mistypes and links to types - #34240

Closed
DerekNonGeneric wants to merge 3 commits into
nodejs:masterfrom
DerekNonGeneric:fix/esm-mistypes-and-links
ConversationCommits (3)ChecksFiles changed
Closed

doc: fix ESM hook mistypes and links to types#34240
DerekNonGeneric wants to merge 3 commits into
nodejs:masterfrom
DerekNonGeneric:fix/esm-mistypes-and-links

Conversation

DerekNonGeneric commented Jul 7, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Prior to this commit, the custom loader hooks were:

  • missing the Node.js API ref docs types
  • missing the function signature from their sections
  • linking directly to the specification (not customary)
  • had an inconsistent non-nullable JSDoc promise return
  • had JSDoc object properties that weren't alpha-sorted
  • designated set of non-nullable types when single was fine
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

nodejs-github-bot added doc Issues and PRs related to the documentations. esm Issues and PRs related to the ECMAScript Modules implementation. labels Jul 7, 2020
addaleax changed the title esm: fix hook mistypes and links to types doc: fix ESM hook mistypes and links to types Jul 7, 2020

addaleax commented Jul 7, 2020

Copy link
Copy Markdown
Member

@nodejs/modules-active-members

DerekNonGeneric commented Jul 7, 2020

Copy link
Copy Markdown
Contributor Author

Credit goes to @devsnek for spotting most of these errors. This PR is also correcting a copying error I had made in a previous PR as well as the removal of two superfluous JSDoc non-nullable type annotations that aren't needed. This PR also corrects oversights that had been made about the document structure that I had not taken notice of. Nothing in this PR should be at all controversial, but rather brings consistency to the document. Rather than having these corrections be packaged into the other PR, my hope is that this one can be fast-tracked and we can focus on the new API being proposed in that other PR, which does contain breaking changes that require more thoughtful consideration.

DerekNonGeneric commented Jul 8, 2020

Copy link
Copy Markdown
Contributor Author

/cc @nodejs/documentation as it's a doc-only change to bring it up to speed with conventions used throughout

DerekNonGeneric commented Jul 12, 2020
View reviewed changes
Comment thread doc/api/esm.md Outdated
Trott reviewed Jul 14, 2020
View reviewed changes
Comment thread doc/api/esm.md Outdated
esm: fix hook mistypes and links to types
50c727b 
Prior to this commit, the custom loader hooks were:
* missing the Node.js API ref docs types
* missing the function signature from their sections
* linking directly to the specification (not customary)
* had an inconsistent non-nullable JSDoc promise return
* had JSDoc object properties that weren't alpha-sorted
* designated set of non-nullable types when single was fine

Notes:
    https://www.typescriptlang.org/play/index.html?strictNullChecks=true&useJavaScript=true#code/PQKhCgAIUgBAHAhgJ0QW0gbwM4BdkCWAdgOYC+k28ApgMYEBmB1yUMCK6WmbkftAeyIATArgJDsALkgBCAILJUATwA8eQqQB8AGl58kyakVwBVAEoAZGbIAUG4iUgAfSAFcR1JkWrCAlHrQkGQUgibUAB64vByoGJgAYh604kIUwl6IbgA2uObU2ALZAG7UMUa4bshE2FgACsgCaATY1KqY7sjZMg6kwVpkbMDgkfACyLiQiNjKRLSQDMmpRJBGhSXU9jT0TCw6kGG4kbj7GQxZufnrpX5YUAeSkx2GxmZWkAC8kEQ52cGfD3CUQA3PdGJBbABZRC4AAWADpUCImrZblpIAAGeEAVluHWAwEgAGUmtRAaJlvD7nwCZAEuNKKTIAzENk-lQ6IxmMhsKcBIy0GTaG48E1INkBCQCPMGAy1kVio4qXwaYT5NkAO6IZS1CpVFaIFYCABGACs6JMBAxIHCybLkPEqt1IOp8I4BsqVXrqncVSqnTIXiYLJZIAB+al+77UDWQENbTm7ZD7INvSx+eGwozWqSRv0+WPxjk7bkZrNeQIqsigviDPjgqEwhFI4Qo26qTE4vGQWnyIgCW3IcliCRET2qyAAdVhxgOrOyjkgAAMzhc8gUFdQl-tbVNkCQ3IKTLVaIbIMayWgBKJdsJ4ZAAJIrOEtSO00+tSBiADktUQwlEPpDQHGch2KVk3DtBk0BhWhYUXMIKVHVlIFGcZcGwcdVmoSofVXHJ12uTZiy5PZfSjeFKMOY5Kz9RCR0kGQAG1KPhaiojYoQkMkfZv2AwcAFp6OWb8AF1aOCPwa2Ce5aQAES8FgbX5AA5a9qHhU1antKY2WZQdKG2UieU9b0Vnwy4Nw2BMSzI9iTkgCzCM3KTwDIIA
    https://closure-compiler.appspot.com/home#code%3D%252F%252F%2520%253D%253DClosureCompiler%253D%253D%250A%252F%252F%2520%2540compilation_level%2520ADVANCED_OPTIMIZATIONS%250A%252F%252F%2520%2540warning_level%2520VERBOSE%250A%252F%252F%2520%2540jscomp_error%2520strictCheckTypes%250A%252F%252F%2520%2540language_out%2520ECMASCRIPT_NEXT%250A%252F%252F%2520%2540checks_only%250A%252F%252F%2520%253D%253D%252FClosureCompiler%253D%253D%250A%250A%252F**%250A%2520*%2520%2540param%2520%257Bstring%257D%2520specifier%250A%2520*%2520%2540param%2520%257B%257B%250A%2520*%2520%2520%2520conditions%253A%2520!Array%253Cstring%253E%252C%250A%2520*%2520%2520%2520parentURL%253A%2520!(string%2520%257C%2520undefined)%252C%250A%2520*%2520%257D%257D%2520context%250A%2520*%2520%2540param%2520%257BFunction%257D%2520defaultResolve%250A%2520*%2520%2540returns%2520%257BPromise%253C%257B%2520url%253A%2520string%2520%257D%253E%257D%250A%2520*%252F%250Aexport%2520async%2520function%2520resolve(specifier%252C%2520context%252C%2520defaultResolve)%2520%257B%250A%2520%2520const%2520%257B%2520parentURL%2520%253D%2520null%2520%257D%2520%253D%2520context%253B%250A%2520%2520if%2520(Math.random()%2520%253E%25200.5)%2520%257B%2520%252F%252F%2520Some%2520condition.%250A%2520%2520%2520%2520%252F%252F%2520For%2520some%2520or%2520all%2520specifiers%252C%2520do%2520some%2520custom%2520logic%2520for%2520resolving.%250A%2520%2520%2520%2520%252F%252F%2520Always%2520return%2520an%2520object%2520of%2520the%2520form%2520%257Burl%253A%2520%253Cstring%253E%257D.%250A%2520%2520%2520%2520return%2520%257B%250A%2520%2520%2520%2520%2520%2520url%253A%2520parentURL%2520%253F%250A%2520%2520%2520%2520%2520%2520%2520%2520new%2520URL(specifier%252C%2520parentURL).href%2520%253A%250A%2520%2520%2520%2520%2520%2520%2520%2520new%2520URL(specifier).href%252C%250A%2520%2520%2520%2520%257D%253B%250A%2520%2520%257D%250A%2520%2520if%2520(Math.random()%2520%253C%25200.5)%2520%257B%2520%252F%252F%2520Another%2520condition.%250A%2520%2520%2520%2520%252F%252F%2520When%2520calling%2520%2560defaultResolve%2560%252C%2520the%2520arguments%2520can%2520be%2520modified.%2520In%2520this%250A%2520%2520%2520%2520%252F%252F%2520case%2520it's%2520adding%2520another%2520value%2520for%2520matching%2520conditional%2520exports.%250A%2520%2520%2520%2520return%2520defaultResolve(specifier%252C%2520%257B%250A%2520%2520%2520%2520%2520%2520...context%252C%250A%2520%2520%2520%2520%2520%2520conditions%253A%2520%255B...context.conditions%252C%2520'another-condition'%255D%252C%250A%2520%2520%2520%2520%257D)%253B%250A%2520%2520%257D%250A%2520%2520%252F%252F%2520Defer%2520to%2520Node.js%2520for%2520all%2520other%2520specifiers.%250A%2520%2520return%2520defaultResolve(specifier%252C%2520context%252C%2520defaultResolve)%253B%250A%257D
DerekNonGeneric commented Jul 15, 2020
View reviewed changes

DerekNonGeneric left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Question about inconsistencies between the Node.js API ref. docs types and the JSDoc type annotations from the snippets below them: how are async functions supposed to be documented? In this PR, the Node.js API ref. docs types for these functions are slight modifications of the ones from the other PR (#33812), but I'm not sure if those were 100% correct.

/cc @devsnek @Trott @GeoffreyBooth

Comment thread doc/api/esm.md Outdated
* `conditions` {string[]}
* `parentURL` {string}
* `defaultResolve` {Function}
* Returns: {Promise}

DerekNonGeneric Jul 15, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

For async functions, do the Node.js API ref. docs types usually express what the promise is wrapping? Compare to the JSDoc below it.

DerekNonGeneric Jul 19, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Update: AFAIK, the Node.js API ref. docs type system lacks the ability to express what we want here. So, since this API is not strictly Promise-based (it works whether this is an async function or not), IMO this should be expressed as an Object the same way the others are.

Suggested change
* Returns: {Promise}
* Returns: {Object}
* `url` {string}

DerekNonGeneric Jul 19, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yet to be documented:

Whether or not this hook should ever be written as a non-async function depends on the circumstance.

Unless others feel strongly about this or have a better suggestion, I'll take the suggestion I've proposed above. The point of this PR was that it was supposed to be as frictionless as possible, so being stuck on this seems counterproductive especially since we would want these changes made before continuing w/ #33812.

DerekNonGeneric Jul 19, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

There's some additional useful information in the thread where the following was mentioned.

if you know your value will be available synchronously don't use a promise? the entire point of promises is "value will be available at some point that I have no inference or control of and my code should still operate consistently through that"
— https://twitter.com/devsnek/status/1284528435671465988

I'm willing to continue the discussion as it relates to this PR, so please feel free to chime if interested.

Trott commented Jul 16, 2020

Copy link
Copy Markdown
Member

Question about inconsistencies between the Node.js API ref. docs types and the JSDoc type annotations from the snippets below them: how are async functions supposed to be documented? In this PR, the Node.js API ref. docs types for these functions are slight modifications of the ones from the other PR (#33812), but I'm not sure if those were 100% correct.

/cc @devsnek @Trott @GeoffreyBooth

@nodejs/documentation

DerekNonGeneric commented Jul 22, 2020
View reviewed changes
Comment thread doc/api/esm.md
* }} context
* @param {Function} defaultResolve
* @returns {!(Promise<{ url: string }>)}
* @returns {Promise<{ url: string }>}

DerekNonGeneric Jul 22, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Justification for this change is as follows.

In strict null checking mode, the null and undefined values are not in the domain of every type and are only assignable to themselves and any (the one exception being that undefined is also assignable to void).
— https://www.typescriptlang.org/docs/handbook/compiler-options.html

The playground link in the commit message confirms silent compilation.

DerekNonGeneric commented Jul 26, 2020

Copy link
Copy Markdown
Contributor Author

+1 to label:"author ready"

Trott added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jul 29, 2020

Trott commented Jul 29, 2020

Copy link
Copy Markdown
Member

@nodejs/modules-active-members Any chance someone can give this a review? It technically can land with just my approval, but it would be nice to get a thumbs up from someone who is more of a subject-matter expert here.

Trott pushed a commit that referenced this pull request Aug 1, 2020
esm: fix hook mistypes and links to types
45fa216 
Prior to this commit, the custom loader hooks were:
* missing the Node.js API ref docs types
* missing the function signature from their sections
* linking directly to the specification (not customary)
* had an inconsistent non-nullable JSDoc promise return
* had JSDoc object properties that weren't alpha-sorted
* designated set of non-nullable types when single was fine

Notes:
    https://www.typescriptlang.org/play/index.html?strictNullChecks=true&useJavaScript=true#code/PQKhCgAIUgBAHAhgJ0QW0gbwM4BdkCWAdgOYC+k28ApgMYEBmB1yUMCK6WmbkftAeyIATArgJDsALkgBCAILJUATwA8eQqQB8AGl58kyakVwBVAEoAZGbIAUG4iUgAfSAFcR1JkWrCAlHrQkGQUgibUAB64vByoGJgAYh604kIUwl6IbgA2uObU2ALZAG7UMUa4bshE2FgACsgCaATY1KqY7sjZMg6kwVpkbMDgkfACyLiQiNjKRLSQDMmpRJBGhSXU9jT0TCw6kGG4kbj7GQxZufnrpX5YUAeSkx2GxmZWkAC8kEQ52cGfD3CUQA3PdGJBbABZRC4AAWADpUCImrZblpIAAGeEAVluHWAwEgAGUmtRAaJlvD7nwCZAEuNKKTIAzENk-lQ6IxmMhsKcBIy0GTaG48E1INkBCQCPMGAy1kVio4qXwaYT5NkAO6IZS1CpVFaIFYCABGACs6JMBAxIHCybLkPEqt1IOp8I4BsqVXrqncVSqnTIXiYLJZIAB+al+77UDWQENbTm7ZD7INvSx+eGwozWqSRv0+WPxjk7bkZrNeQIqsigviDPjgqEwhFI4Qo26qTE4vGQWnyIgCW3IcliCRET2qyAAdVhxgOrOyjkgAAMzhc8gUFdQl-tbVNkCQ3IKTLVaIbIMayWgBKJdsJ4ZAAJIrOEtSO00+tSBiADktUQwlEPpDQHGch2KVk3DtBk0BhWhYUXMIKVHVlIFGcZcGwcdVmoSofVXHJ12uTZiy5PZfSjeFKMOY5Kz9RCR0kGQAG1KPhaiojYoQkMkfZv2AwcAFp6OWb8AF1aOCPwa2Ce5aQAES8FgbX5AA5a9qHhU1antKY2WZQdKG2UieU9b0Vnwy4Nw2BMSzI9iTkgCzCM3KTwDIIA
    https://closure-compiler.appspot.com/home#code%3D%252F%252F%2520%253D%253DClosureCompiler%253D%253D%250A%252F%252F%2520%2540compilation_level%2520ADVANCED_OPTIMIZATIONS%250A%252F%252F%2520%2540warning_level%2520VERBOSE%250A%252F%252F%2520%2540jscomp_error%2520strictCheckTypes%250A%252F%252F%2520%2540language_out%2520ECMASCRIPT_NEXT%250A%252F%252F%2520%2540checks_only%250A%252F%252F%2520%253D%253D%252FClosureCompiler%253D%253D%250A%250A%252F**%250A%2520*%2520%2540param%2520%257Bstring%257D%2520specifier%250A%2520*%2520%2540param%2520%257B%257B%250A%2520*%2520%2520%2520conditions%253A%2520!Array%253Cstring%253E%252C%250A%2520*%2520%2520%2520parentURL%253A%2520!(string%2520%257C%2520undefined)%252C%250A%2520*%2520%257D%257D%2520context%250A%2520*%2520%2540param%2520%257BFunction%257D%2520defaultResolve%250A%2520*%2520%2540returns%2520%257BPromise%253C%257B%2520url%253A%2520string%2520%257D%253E%257D%250A%2520*%252F%250Aexport%2520async%2520function%2520resolve(specifier%252C%2520context%252C%2520defaultResolve)%2520%257B%250A%2520%2520const%2520%257B%2520parentURL%2520%253D%2520null%2520%257D%2520%253D%2520context%253B%250A%2520%2520if%2520(Math.random()%2520%253E%25200.5)%2520%257B%2520%252F%252F%2520Some%2520condition.%250A%2520%2520%2520%2520%252F%252F%2520For%2520some%2520or%2520all%2520specifiers%252C%2520do%2520some%2520custom%2520logic%2520for%2520resolving.%250A%2520%2520%2520%2520%252F%252F%2520Always%2520return%2520an%2520object%2520of%2520the%2520form%2520%257Burl%253A%2520%253Cstring%253E%257D.%250A%2520%2520%2520%2520return%2520%257B%250A%2520%2520%2520%2520%2520%2520url%253A%2520parentURL%2520%253F%250A%2520%2520%2520%2520%2520%2520%2520%2520new%2520URL(specifier%252C%2520parentURL).href%2520%253A%250A%2520%2520%2520%2520%2520%2520%2520%2520new%2520URL(specifier).href%252C%250A%2520%2520%2520%2520%257D%253B%250A%2520%2520%257D%250A%2520%2520if%2520(Math.random()%2520%253C%25200.5)%2520%257B%2520%252F%252F%2520Another%2520condition.%250A%2520%2520%2520%2520%252F%252F%2520When%2520calling%2520%2560defaultResolve%2560%252C%2520the%2520arguments%2520can%2520be%2520modified.%2520In%2520this%250A%2520%2520%2520%2520%252F%252F%2520case%2520it's%2520adding%2520another%2520value%2520for%2520matching%2520conditional%2520exports.%250A%2520%2520%2520%2520return%2520defaultResolve(specifier%252C%2520%257B%250A%2520%2520%2520%2520%2520%2520...context%252C%250A%2520%2520%2520%2520%2520%2520conditions%253A%2520%255B...context.conditions%252C%2520'another-condition'%255D%252C%250A%2520%2520%2520%2520%257D)%253B%250A%2520%2520%257D%250A%2520%2520%252F%252F%2520Defer%2520to%2520Node.js%2520for%2520all%2520other%2520specifiers.%250A%2520%2520return%2520defaultResolve(specifier%252C%2520context%252C%2520defaultResolve)%253B%250A%257D

PR-URL: #34240
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>

Trott commented Aug 1, 2020

Copy link
Copy Markdown
Member

Landed in 45fa216

Trott closed this Aug 1, 2020
codebytere pushed a commit that referenced this pull request Aug 5, 2020
esm: fix hook mistypes and links to types
44e6c01 
Prior to this commit, the custom loader hooks were:
* missing the Node.js API ref docs types
* missing the function signature from their sections
* linking directly to the specification (not customary)
* had an inconsistent non-nullable JSDoc promise return
* had JSDoc object properties that weren't alpha-sorted
* designated set of non-nullable types when single was fine

Notes:
    https://www.typescriptlang.org/play/index.html?strictNullChecks=true&useJavaScript=true#code/PQKhCgAIUgBAHAhgJ0QW0gbwM4BdkCWAdgOYC+k28ApgMYEBmB1yUMCK6WmbkftAeyIATArgJDsALkgBCAILJUATwA8eQqQB8AGl58kyakVwBVAEoAZGbIAUG4iUgAfSAFcR1JkWrCAlHrQkGQUgibUAB64vByoGJgAYh604kIUwl6IbgA2uObU2ALZAG7UMUa4bshE2FgACsgCaATY1KqY7sjZMg6kwVpkbMDgkfACyLiQiNjKRLSQDMmpRJBGhSXU9jT0TCw6kGG4kbj7GQxZufnrpX5YUAeSkx2GxmZWkAC8kEQ52cGfD3CUQA3PdGJBbABZRC4AAWADpUCImrZblpIAAGeEAVluHWAwEgAGUmtRAaJlvD7nwCZAEuNKKTIAzENk-lQ6IxmMhsKcBIy0GTaG48E1INkBCQCPMGAy1kVio4qXwaYT5NkAO6IZS1CpVFaIFYCABGACs6JMBAxIHCybLkPEqt1IOp8I4BsqVXrqncVSqnTIXiYLJZIAB+al+77UDWQENbTm7ZD7INvSx+eGwozWqSRv0+WPxjk7bkZrNeQIqsigviDPjgqEwhFI4Qo26qTE4vGQWnyIgCW3IcliCRET2qyAAdVhxgOrOyjkgAAMzhc8gUFdQl-tbVNkCQ3IKTLVaIbIMayWgBKJdsJ4ZAAJIrOEtSO00+tSBiADktUQwlEPpDQHGch2KVk3DtBk0BhWhYUXMIKVHVlIFGcZcGwcdVmoSofVXHJ12uTZiy5PZfSjeFKMOY5Kz9RCR0kGQAG1KPhaiojYoQkMkfZv2AwcAFp6OWb8AF1aOCPwa2Ce5aQAES8FgbX5AA5a9qHhU1antKY2WZQdKG2UieU9b0Vnwy4Nw2BMSzI9iTkgCzCM3KTwDIIA
    https://closure-compiler.appspot.com/home#code%3D%252F%252F%2520%253D%253DClosureCompiler%253D%253D%250A%252F%252F%2520%2540compilation_level%2520ADVANCED_OPTIMIZATIONS%250A%252F%252F%2520%2540warning_level%2520VERBOSE%250A%252F%252F%2520%2540jscomp_error%2520strictCheckTypes%250A%252F%252F%2520%2540language_out%2520ECMASCRIPT_NEXT%250A%252F%252F%2520%2540checks_only%250A%252F%252F%2520%253D%253D%252FClosureCompiler%253D%253D%250A%250A%252F**%250A%2520*%2520%2540param%2520%257Bstring%257D%2520specifier%250A%2520*%2520%2540param%2520%257B%257B%250A%2520*%2520%2520%2520conditions%253A%2520!Array%253Cstring%253E%252C%250A%2520*%2520%2520%2520parentURL%253A%2520!(string%2520%257C%2520undefined)%252C%250A%2520*%2520%257D%257D%2520context%250A%2520*%2520%2540param%2520%257BFunction%257D%2520defaultResolve%250A%2520*%2520%2540returns%2520%257BPromise%253C%257B%2520url%253A%2520string%2520%257D%253E%257D%250A%2520*%252F%250Aexport%2520async%2520function%2520resolve(specifier%252C%2520context%252C%2520defaultResolve)%2520%257B%250A%2520%2520const%2520%257B%2520parentURL%2520%253D%2520null%2520%257D%2520%253D%2520context%253B%250A%2520%2520if%2520(Math.random()%2520%253E%25200.5)%2520%257B%2520%252F%252F%2520Some%2520condition.%250A%2520%2520%2520%2520%252F%252F%2520For%2520some%2520or%2520all%2520specifiers%252C%2520do%2520some%2520custom%2520logic%2520for%2520resolving.%250A%2520%2520%2520%2520%252F%252F%2520Always%2520return%2520an%2520object%2520of%2520the%2520form%2520%257Burl%253A%2520%253Cstring%253E%257D.%250A%2520%2520%2520%2520return%2520%257B%250A%2520%2520%2520%2520%2520%2520url%253A%2520parentURL%2520%253F%250A%2520%2520%2520%2520%2520%2520%2520%2520new%2520URL(specifier%252C%2520parentURL).href%2520%253A%250A%2520%2520%2520%2520%2520%2520%2520%2520new%2520URL(specifier).href%252C%250A%2520%2520%2520%2520%257D%253B%250A%2520%2520%257D%250A%2520%2520if%2520(Math.random()%2520%253C%25200.5)%2520%257B%2520%252F%252F%2520Another%2520condition.%250A%2520%2520%2520%2520%252F%252F%2520When%2520calling%2520%2560defaultResolve%2560%252C%2520the%2520arguments%2520can%2520be%2520modified.%2520In%2520this%250A%2520%2520%2520%2520%252F%252F%2520case%2520it's%2520adding%2520another%2520value%2520for%2520matching%2520conditional%2520exports.%250A%2520%2520%2520%2520return%2520defaultResolve(specifier%252C%2520%257B%250A%2520%2520%2520%2520%2520%2520...context%252C%250A%2520%2520%2520%2520%2520%2520conditions%253A%2520%255B...context.conditions%252C%2520'another-condition'%255D%252C%250A%2520%2520%2520%2520%257D)%253B%250A%2520%2520%257D%250A%2520%2520%252F%252F%2520Defer%2520to%2520Node.js%2520for%2520all%2520other%2520specifiers.%250A%2520%2520return%2520defaultResolve(specifier%252C%2520context%252C%2520defaultResolve)%253B%250A%257D

PR-URL: #34240
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
codebytere mentioned this pull request Aug 10, 2020
Merged
MylesBorins pushed a commit that referenced this pull request Nov 3, 2020
esm: fix hook mistypes and links to types
182ba3d 
Prior to this commit, the custom loader hooks were:
* missing the Node.js API ref docs types
* missing the function signature from their sections
* linking directly to the specification (not customary)
* had an inconsistent non-nullable JSDoc promise return
* had JSDoc object properties that weren't alpha-sorted
* designated set of non-nullable types when single was fine

Notes:
    https://www.typescriptlang.org/play/index.html?strictNullChecks=true&useJavaScript=true#code/PQKhCgAIUgBAHAhgJ0QW0gbwM4BdkCWAdgOYC+k28ApgMYEBmB1yUMCK6WmbkftAeyIATArgJDsALkgBCAILJUATwA8eQqQB8AGl58kyakVwBVAEoAZGbIAUG4iUgAfSAFcR1JkWrCAlHrQkGQUgibUAB64vByoGJgAYh604kIUwl6IbgA2uObU2ALZAG7UMUa4bshE2FgACsgCaATY1KqY7sjZMg6kwVpkbMDgkfACyLiQiNjKRLSQDMmpRJBGhSXU9jT0TCw6kGG4kbj7GQxZufnrpX5YUAeSkx2GxmZWkAC8kEQ52cGfD3CUQA3PdGJBbABZRC4AAWADpUCImrZblpIAAGeEAVluHWAwEgAGUmtRAaJlvD7nwCZAEuNKKTIAzENk-lQ6IxmMhsKcBIy0GTaG48E1INkBCQCPMGAy1kVio4qXwaYT5NkAO6IZS1CpVFaIFYCABGACs6JMBAxIHCybLkPEqt1IOp8I4BsqVXrqncVSqnTIXiYLJZIAB+al+77UDWQENbTm7ZD7INvSx+eGwozWqSRv0+WPxjk7bkZrNeQIqsigviDPjgqEwhFI4Qo26qTE4vGQWnyIgCW3IcliCRET2qyAAdVhxgOrOyjkgAAMzhc8gUFdQl-tbVNkCQ3IKTLVaIbIMayWgBKJdsJ4ZAAJIrOEtSO00+tSBiADktUQwlEPpDQHGch2KVk3DtBk0BhWhYUXMIKVHVlIFGcZcGwcdVmoSofVXHJ12uTZiy5PZfSjeFKMOY5Kz9RCR0kGQAG1KPhaiojYoQkMkfZv2AwcAFp6OWb8AF1aOCPwa2Ce5aQAES8FgbX5AA5a9qHhU1antKY2WZQdKG2UieU9b0Vnwy4Nw2BMSzI9iTkgCzCM3KTwDIIA
    https://closure-compiler.appspot.com/home#code%3D%252F%252F%2520%253D%253DClosureCompiler%253D%253D%250A%252F%252F%2520%2540compilation_level%2520ADVANCED_OPTIMIZATIONS%250A%252F%252F%2520%2540warning_level%2520VERBOSE%250A%252F%252F%2520%2540jscomp_error%2520strictCheckTypes%250A%252F%252F%2520%2540language_out%2520ECMASCRIPT_NEXT%250A%252F%252F%2520%2540checks_only%250A%252F%252F%2520%253D%253D%252FClosureCompiler%253D%253D%250A%250A%252F**%250A%2520*%2520%2540param%2520%257Bstring%257D%2520specifier%250A%2520*%2520%2540param%2520%257B%257B%250A%2520*%2520%2520%2520conditions%253A%2520!Array%253Cstring%253E%252C%250A%2520*%2520%2520%2520parentURL%253A%2520!(string%2520%257C%2520undefined)%252C%250A%2520*%2520%257D%257D%2520context%250A%2520*%2520%2540param%2520%257BFunction%257D%2520defaultResolve%250A%2520*%2520%2540returns%2520%257BPromise%253C%257B%2520url%253A%2520string%2520%257D%253E%257D%250A%2520*%252F%250Aexport%2520async%2520function%2520resolve(specifier%252C%2520context%252C%2520defaultResolve)%2520%257B%250A%2520%2520const%2520%257B%2520parentURL%2520%253D%2520null%2520%257D%2520%253D%2520context%253B%250A%2520%2520if%2520(Math.random()%2520%253E%25200.5)%2520%257B%2520%252F%252F%2520Some%2520condition.%250A%2520%2520%2520%2520%252F%252F%2520For%2520some%2520or%2520all%2520specifiers%252C%2520do%2520some%2520custom%2520logic%2520for%2520resolving.%250A%2520%2520%2520%2520%252F%252F%2520Always%2520return%2520an%2520object%2520of%2520the%2520form%2520%257Burl%253A%2520%253Cstring%253E%257D.%250A%2520%2520%2520%2520return%2520%257B%250A%2520%2520%2520%2520%2520%2520url%253A%2520parentURL%2520%253F%250A%2520%2520%2520%2520%2520%2520%2520%2520new%2520URL(specifier%252C%2520parentURL).href%2520%253A%250A%2520%2520%2520%2520%2520%2520%2520%2520new%2520URL(specifier).href%252C%250A%2520%2520%2520%2520%257D%253B%250A%2520%2520%257D%250A%2520%2520if%2520(Math.random()%2520%253C%25200.5)%2520%257B%2520%252F%252F%2520Another%2520condition.%250A%2520%2520%2520%2520%252F%252F%2520When%2520calling%2520%2560defaultResolve%2560%252C%2520the%2520arguments%2520can%2520be%2520modified.%2520In%2520this%250A%2520%2520%2520%2520%252F%252F%2520case%2520it's%2520adding%2520another%2520value%2520for%2520matching%2520conditional%2520exports.%250A%2520%2520%2520%2520return%2520defaultResolve(specifier%252C%2520%257B%250A%2520%2520%2520%2520%2520%2520...context%252C%250A%2520%2520%2520%2520%2520%2520conditions%253A%2520%255B...context.conditions%252C%2520'another-condition'%255D%252C%250A%2520%2520%2520%2520%257D)%253B%250A%2520%2520%257D%250A%2520%2520%252F%252F%2520Defer%2520to%2520Node.js%2520for%2520all%2520other%2520specifiers.%250A%2520%2520return%2520defaultResolve(specifier%252C%2520context%252C%2520defaultResolve)%253B%250A%257D

PR-URL: #34240
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
MylesBorins mentioned this pull request Nov 3, 2020
Merged
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
esm: fix hook mistypes and links to types
7a98961 
Prior to this commit, the custom loader hooks were:
* missing the Node.js API ref docs types
* missing the function signature from their sections
* linking directly to the specification (not customary)
* had an inconsistent non-nullable JSDoc promise return
* had JSDoc object properties that weren't alpha-sorted
* designated set of non-nullable types when single was fine

Notes:
    https://www.typescriptlang.org/play/index.html?strictNullChecks=true&useJavaScript=true#code/PQKhCgAIUgBAHAhgJ0QW0gbwM4BdkCWAdgOYC+k28ApgMYEBmB1yUMCK6WmbkftAeyIATArgJDsALkgBCAILJUATwA8eQqQB8AGl58kyakVwBVAEoAZGbIAUG4iUgAfSAFcR1JkWrCAlHrQkGQUgibUAB64vByoGJgAYh604kIUwl6IbgA2uObU2ALZAG7UMUa4bshE2FgACsgCaATY1KqY7sjZMg6kwVpkbMDgkfACyLiQiNjKRLSQDMmpRJBGhSXU9jT0TCw6kGG4kbj7GQxZufnrpX5YUAeSkx2GxmZWkAC8kEQ52cGfD3CUQA3PdGJBbABZRC4AAWADpUCImrZblpIAAGeEAVluHWAwEgAGUmtRAaJlvD7nwCZAEuNKKTIAzENk-lQ6IxmMhsKcBIy0GTaG48E1INkBCQCPMGAy1kVio4qXwaYT5NkAO6IZS1CpVFaIFYCABGACs6JMBAxIHCybLkPEqt1IOp8I4BsqVXrqncVSqnTIXiYLJZIAB+al+77UDWQENbTm7ZD7INvSx+eGwozWqSRv0+WPxjk7bkZrNeQIqsigviDPjgqEwhFI4Qo26qTE4vGQWnyIgCW3IcliCRET2qyAAdVhxgOrOyjkgAAMzhc8gUFdQl-tbVNkCQ3IKTLVaIbIMayWgBKJdsJ4ZAAJIrOEtSO00+tSBiADktUQwlEPpDQHGch2KVk3DtBk0BhWhYUXMIKVHVlIFGcZcGwcdVmoSofVXHJ12uTZiy5PZfSjeFKMOY5Kz9RCR0kGQAG1KPhaiojYoQkMkfZv2AwcAFp6OWb8AF1aOCPwa2Ce5aQAES8FgbX5AA5a9qHhU1antKY2WZQdKG2UieU9b0Vnwy4Nw2BMSzI9iTkgCzCM3KTwDIIA
    https://closure-compiler.appspot.com/home#code%3D%252F%252F%2520%253D%253DClosureCompiler%253D%253D%250A%252F%252F%2520%2540compilation_level%2520ADVANCED_OPTIMIZATIONS%250A%252F%252F%2520%2540warning_level%2520VERBOSE%250A%252F%252F%2520%2540jscomp_error%2520strictCheckTypes%250A%252F%252F%2520%2540language_out%2520ECMASCRIPT_NEXT%250A%252F%252F%2520%2540checks_only%250A%252F%252F%2520%253D%253D%252FClosureCompiler%253D%253D%250A%250A%252F**%250A%2520*%2520%2540param%2520%257Bstring%257D%2520specifier%250A%2520*%2520%2540param%2520%257B%257B%250A%2520*%2520%2520%2520conditions%253A%2520!Array%253Cstring%253E%252C%250A%2520*%2520%2520%2520parentURL%253A%2520!(string%2520%257C%2520undefined)%252C%250A%2520*%2520%257D%257D%2520context%250A%2520*%2520%2540param%2520%257BFunction%257D%2520defaultResolve%250A%2520*%2520%2540returns%2520%257BPromise%253C%257B%2520url%253A%2520string%2520%257D%253E%257D%250A%2520*%252F%250Aexport%2520async%2520function%2520resolve(specifier%252C%2520context%252C%2520defaultResolve)%2520%257B%250A%2520%2520const%2520%257B%2520parentURL%2520%253D%2520null%2520%257D%2520%253D%2520context%253B%250A%2520%2520if%2520(Math.random()%2520%253E%25200.5)%2520%257B%2520%252F%252F%2520Some%2520condition.%250A%2520%2520%2520%2520%252F%252F%2520For%2520some%2520or%2520all%2520specifiers%252C%2520do%2520some%2520custom%2520logic%2520for%2520resolving.%250A%2520%2520%2520%2520%252F%252F%2520Always%2520return%2520an%2520object%2520of%2520the%2520form%2520%257Burl%253A%2520%253Cstring%253E%257D.%250A%2520%2520%2520%2520return%2520%257B%250A%2520%2520%2520%2520%2520%2520url%253A%2520parentURL%2520%253F%250A%2520%2520%2520%2520%2520%2520%2520%2520new%2520URL(specifier%252C%2520parentURL).href%2520%253A%250A%2520%2520%2520%2520%2520%2520%2520%2520new%2520URL(specifier).href%252C%250A%2520%2520%2520%2520%257D%253B%250A%2520%2520%257D%250A%2520%2520if%2520(Math.random()%2520%253C%25200.5)%2520%257B%2520%252F%252F%2520Another%2520condition.%250A%2520%2520%2520%2520%252F%252F%2520When%2520calling%2520%2560defaultResolve%2560%252C%2520the%2520arguments%2520can%2520be%2520modified.%2520In%2520this%250A%2520%2520%2520%2520%252F%252F%2520case%2520it's%2520adding%2520another%2520value%2520for%2520matching%2520conditional%2520exports.%250A%2520%2520%2520%2520return%2520defaultResolve(specifier%252C%2520%257B%250A%2520%2520%2520%2520%2520%2520...context%252C%250A%2520%2520%2520%2520%2520%2520conditions%253A%2520%255B...context.conditions%252C%2520'another-condition'%255D%252C%250A%2520%2520%2520%2520%257D)%253B%250A%2520%2520%257D%250A%2520%2520%252F%252F%2520Defer%2520to%2520Node.js%2520for%2520all%2520other%2520specifiers.%250A%2520%2520return%2520defaultResolve(specifier%252C%2520context%252C%2520defaultResolve)%253B%250A%257D

PR-URL: #34240
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott approved these changes
guybedford guybedford approved these changes
+1 more reviewer
bmeck bmeck left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. doc Issues and PRs related to the documentations. esm Issues and PRs related to the ECMAScript Modules implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL