Skip to content

Docs: Add Iterative chaining design #32

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

Merged
merged 13 commits into from
Nov 3, 2021

Conversation

JakobJingleheimer
Copy link
Member

No description provided.

@GeoffreyBooth

This comment has been minimized.

@JakobJingleheimer

This comment has been minimized.

@GeoffreyBooth

This comment has been minimized.

@GeoffreyBooth GeoffreyBooth changed the title Update design docs to support multiple concurrent proposals Add “iterative” chaining design Sep 30, 2021
@GeoffreyBooth GeoffreyBooth changed the title Add “iterative” chaining design Iterative chaining design Sep 30, 2021
@GeoffreyBooth
Copy link
Member

@JakobJingleheimer do you mind splitting out the updates to the recursive design to be its own PR?

@JakobJingleheimer
Copy link
Member Author

Sure. I'm away til Sunday; I can do it then.

@GeoffreyBooth GeoffreyBooth added the loaders-agenda Issues and PRs to discuss during the meetings of the Loaders team label Oct 20, 2021
format?: string, // a hint to the load hook (it can be ignored)
shortCircuit?: true, // signal that this hook intends to terminate the
// `resolve` chain
url: string, // the final hook must return a valid URL string
Copy link
Member

Choose a reason for hiding this comment

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

I suppose all these are optional, since a hook could just return void; and then the loader would be skipped, right? And if none returned anything, Node’s default eventually would?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, merely returning void would cause that specific loader to be skipped. If all loaders did that, Node's default would eventually run.


* `unpkg-resolver` converts a bare specifier like `foo` to a url `http://unpkg.com/foo`.
1. `http-to-https-resolver` rewrites insecure http urls to the https, like `https://unpkg.com/foo`.
1. `cache-buster-resolver` adds a timestamp to a url end, like `https://unpkg.com/foo?t=1234567890`.
Copy link
Member

Choose a reason for hiding this comment

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

So in the current docs “transpiler loader” example, the full “loader” is a set of one resolve hook and one load hook. So coffeescript-loader would be the set of a resolve hook and a load hook, not a reference to just that loader’s load hook. Hence -resolver feels weird, as -loader is not its parallel.

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the doc to strip those out in all but some file names. The casual nomenclature I was using was a resolver provides only a resolve hook, and a loader (an unfortunate naming collision with the main "loader" concept) provides both resolve and load hooks. I dunno what one might be called that provides only a load hook—maybe "sourcer"? But taking it out hopefully means we don't have to solve that problem 😛

For this casual usage:

  • "resolvers" would be: unpkg, cache-buster
  • "loaders" would be: coffeescript, https, and maybe babel

interimResult: { // result from the previous hook
format = '', // most recently provided value from a previous hook
url = '', // most recently provided value from a previous hook
},
Copy link
Member

Choose a reason for hiding this comment

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

I would think that this should be undefined by default, not an object with empty strings as values. That’s really awkward to test against.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I considered that.

I think resolve and load's interimResult should be the same in that regard, and resolve may have settled with a format, which is fed into load's interimResult.format, so then undefined vs quasi-empty object gets rather complicated. So I figured it's most straight-forward to just make them both objects with falsy initial property values of the appropriate data-type.

conditions = string[], // export conditions from the relevant package.json
parentUrl = null, // foo.mjs imports bar.mjs
// when module is bar.mjs, parentUrl is foo.mjs
originalSpecifier, // The original value of the import specifier
Copy link
Member

Choose a reason for hiding this comment

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

Why not just specifier? There’s no other specifier parameter, and since it’s not part of interimResult it would appear that no hook can modify the specifier.

This is a key difference between the designs: in the other design, one loader can call the next with different values for all of the parameters. In this design, a loader can communicate to the next in the chain only via its return values.

Copy link
Member Author

Choose a reason for hiding this comment

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

🤔 I went with originalSpecifier to ensure it was obvious that that value has not been modified in any way from what the user provided, and thus is not to be confused with the settled url. I know people are already confused by the term specifier and don't use it (using almost exclusively "import path" or "import url"), so it seemed like something that needed some extra clarity.

But thinking on it now, the properties in context are all "original"-ish values, so maybe it does make sense to just call it specifier.

export async function resolve(
interimResult,
) {
const url = new URL(interimResult.url); // this can throw, so handle appropriately
Copy link
Member

Choose a reason for hiding this comment

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

And if this loader is first, so there’s no interimResult yet? Something like this?

if (!interimResult) {
  interimResult = defaultResolve(undefined, context);
}

const url = new URL(interimResult.url);

And now we have a hybrid of the two designs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes? I think that only applies here (or to a first one) though?

},
context: {
conditions, // export conditions from the relevant package.json
parentUrl, // foo.mjs imports bar.mjs
Copy link
Member

Choose a reason for hiding this comment

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

Is this intended to be here? If it is, shouldn’t specifier be here too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this intended to be here?

parentUrl? Yes, I think it's supposed to be there.

shouldn’t specifier be here too

No? 🤔 It could be, but why? (this is load, so specifier has already settled to url)

Comment on lines 39 to 43
signals?: { // signals from this hook to the ESMLoader
contextOverride?: object, // the new `context` argument for the next hook
interimIgnored?: true, // interimResult was intentionally ignored
shortCircuit?: true, // `resolve` chain should be terminated
},
Copy link
Member Author

Choose a reason for hiding this comment

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

I thought of this after the meeting whilst I was updating the docs: nesting these under signals seems like a nice grouping to denote they are all signals, and reduces the surface area of the return (which was growing rapidly).

@JakobJingleheimer JakobJingleheimer changed the title Iterative chaining design Docs: Add Iterative chaining design Nov 3, 2021
@JakobJingleheimer JakobJingleheimer merged commit 3a9e140 into nodejs:main Nov 3, 2021
@JakobJingleheimer JakobJingleheimer deleted the update-design-docs branch November 3, 2021 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
loaders-agenda Issues and PRs to discuss during the meetings of the Loaders team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants