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

Move chaining todo item to "completed" #77

Merged
merged 1 commit into from May 9, 2022

Conversation

JakobJingleheimer
Copy link
Contributor

No description provided.

@JakobJingleheimer
Copy link
Contributor Author

JakobJingleheimer commented May 5, 2022

Should we also add these to the Upcoming list?

  • "Move loaders off thread"
  • "Convert resolve from async to sync"
    • and perhaps an accompanying "Add async resolve to module module"?

@arcanis
Copy link
Contributor

arcanis commented May 5, 2022

Do you think, with the chaining being merged, it's worth discussing ambient loaders (and perhaps "should ambient loaders just be loaders", as I recall @cspotcode suggested it and we didn't recall why it'd be best avoided)? I would like to try to make a prototype soonish, if there's consensus on the general design.

### In Progress

### Upcoming

1. Implement chaining as described in the [design](doc/design/proposal-chaining-middleware.md), where the `default<hookName>` becomes `next` and references the next registered hook in the chain.

1. Get a `load` return value of `format: 'commonjs'` to work, or at least error informatively. See https://github.com/nodejs/node/issues/34753#issuecomment-735921348.
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this already done too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think no because ESMLoader can't talk to CJSLoader or something. We could add an error (or maybe a warning?) though.

Copy link
Member

Choose a reason for hiding this comment

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

If you or someone has time, we should confirm this one way or the other. I think I thought it was broken but then the test CoffeeScript loader got fixed, which proves that this wasn't an issue (but it was an author error on my part, which suggests maybe we need a better error message).

@JakobJingleheimer JakobJingleheimer merged commit 6c281b0 into main May 9, 2022
@JakobJingleheimer JakobJingleheimer deleted the move-chaining-to-complete branch May 9, 2022 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants