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

esm: update example-loader test fixture & API docs #29819

Closed
wants to merge 2 commits into from
Closed

esm: update example-loader test fixture & API docs #29819

wants to merge 2 commits into from

Conversation

DerekNonGeneric
Copy link
Contributor

@DerekNonGeneric DerekNonGeneric commented Oct 2, 2019

  • Remove importing of URL & process as is unnecessary (they exist in global scope)
  • Correct all type annotations (param & returns) to be accurate to both TypeScript & Closure
  • Fix bare-import check error message to be accurate and grammatically correct
  • Update bare-import check to more closely match language used in error message
  • Update command that runs the example to be cross-platform
  • Prefer use of captialized {Function} type annotation over {function}
  • Update prose regarding the condition in which parentModuleURL is undefined
  • Move assignment of parentModuleURL to inside the resolve hook function
  • Prefer --experimental-loader to --loader (taken care of in another PR)

Fixes: #29610

Checklist

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Oct 2, 2019
@DerekNonGeneric
Copy link
Contributor Author

@bmeck, WDYT?

doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented Oct 11, 2019

@nodejs/modules-active-members @nodejs/documentation This could use some reviews.

@Trott
Copy link
Member

Trott commented Oct 13, 2019

@nodejs/collaborators This could use some reviews.

@DerekNonGeneric
Copy link
Contributor Author

DerekNonGeneric commented Oct 15, 2019

@Trott, thanks for reaching out to everyone, the topic of type checking .mjs files using TypeScript/Closure compiler-style JSDoc comments seems to be a little more involved than I initially thought. At this point in my continuing research on the topic, I'd like to caution people who are unfamiliar with it and happen to arrive at this PR eager to help. Ensuring that JSDoc type annotations are interoperable between the TypeScript and Closure type systems is not a small feat to accomplish. There are some odd considerations that need to be made, which I intend on addressing. I have a good solution that I hope to demonstrate in an example repo. As such, I recommend holding off on further revision until it's ready.

@Trott Trott added the wip Issues and PRs that are still a work in progress. label Oct 15, 2019
@DerekNonGeneric
Copy link
Contributor Author

DerekNonGeneric commented Oct 28, 2019

As promised, I have created the demonstration repo. It would be great if everyone involved could take a brief moment to ensure that the compilers have their options set correctly. This takes place in tasks/typecheck-sources.js. Thanks in advance! The solution proposed in the PR will be kept up to date on the dummy-loader-solution branch for anyone interested in playing with it.

/to @bmeck @guybedford @jkrems @weswigham @devsnek @vsemozhetbyt @BridgeAR

@DerekNonGeneric DerekNonGeneric changed the title doc: update Resolve Hook examples module: update example-loader test fixture & ESM API docs Oct 30, 2019
@DerekNonGeneric DerekNonGeneric changed the title module: update example-loader test fixture & ESM API docs esm: update example-loader test fixture & API docs Oct 30, 2019
@DerekNonGeneric
Copy link
Contributor Author

DerekNonGeneric commented Oct 30, 2019

I'd like to bring to the attention of those designing the resolver hook API what is probably the most significant finding of this whole effort: the order of the parameters is not ideal for Closure. As it currently stands, Closure emits the following warning:

WARNING - [JSC_OPTIONAL_ARG_AT_END] optional arguments must be at the end
export async function resolve(specifier, parentModuleURL = undefined, defaultResolver) {
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

0 error(s), 1 warning(s), 83.9% typed

Apparently, Closure wants the optional param to be last. It might be best to change this while we still can if there's no justification for keeping the current order.

Copy link
Contributor

@jkrems jkrems left a comment

Choose a reason for hiding this comment

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

LGTM in general

doc/api/esm.md Outdated Show resolved Hide resolved
@DerekNonGeneric
Copy link
Contributor Author

DerekNonGeneric commented Dec 20, 2019

/to @nodejs/modules-active-members

Other than the merge conflict, is anything blocking this?

@GeoffreyBooth
Copy link
Member

We should land this with #30986, which also refactors resolve.

@DerekNonGeneric
Copy link
Contributor Author

@GeoffreyBooth, how would you recommend we do this? Should I make a PR to your fork?

@GeoffreyBooth
Copy link
Member

GeoffreyBooth commented Dec 22, 2019

Cc @bmeck or @guybedford for opinions.

Does this PR have any conflicts with that one? If not yeah, I guess you can PR my branch and then this work becomes part of that PR. Anyone object to this plan?

@DerekNonGeneric
Copy link
Contributor Author

No word from @bmeck or @guybedford. Is applying types to the code samples still desired?

@GeoffreyBooth
Copy link
Member

I think it's desired, can you rebase?

@HarshithaKP
Copy link
Member

@DerekNonGeneric, this needs a rebase.

@DerekNonGeneric
Copy link
Contributor Author

Glad to see there's still interest in this. There are a few reasons why a rebase of this PR hasn't happened yet and some of the discussion about it has happened outside of this thread. Allow me to bring folks up to speed.

  1. When this PR was made, there was only one loader sample on the docs page.
  2. This code sample was also a test fixture, which made it easy to pull in/import/embed.
  3. New hooks have been added to the custom loader API along with several more code samples.
  4. These new hook samples do not have corresponding fixtures in the test directory.

I believe @GeoffreyBooth had mentioned that he had the new hook samples as individual files during a sync we had. I'm still unclear whether fixtures can play this dual role as samples or if they should live in a separate directory or even repo. I got a new email address and sent out a hangouts invitation—happy to set up another sync or continue the conversation. Please keep me posted.

* Remove import of URL & process as they are unnecessary
* Improve accuracy of hook's thrown error message
* Improve accuracy of parameter & return type annotations
* Move assignment of default parameter to inside hook function
@DerekNonGeneric
Copy link
Contributor Author

DerekNonGeneric commented Apr 3, 2020

/to @HarshithaKP

@DerekNonGeneric, this needs a rebase.

With my previous comment in mind, I've rebased the PR to continue in the spirit of the original PR. This fixture no longer exists as a sample in the docs, so hopes of embedding it as one are probably lost. I was not the one who added the new samples, but I will note that they are neither valid TypeScript Compiler nor Closure Compiler annotations. Fixing all of them in this PR would beyond its scope, though. There are also two larger examples (the https loader and transpiler loader) that need work.

Ideally these samples would live in their own files and be subject to typechecking and linting prior to being embedded into the docs via a templating engine otherwise you inevitably end up providing bad samples to users, which isn't uncommon.

@DerekNonGeneric
Copy link
Contributor Author

Closing in favor of #32646 as this branch name is no longer relevant.

@DerekNonGeneric DerekNonGeneric deleted the patch-1 branch April 3, 2020 21:51
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. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

doc: ES module dummy loader resolve hook bug on Windows