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: fix http(s) import via custom loader #43130

Conversation

JakobJingleheimer
Copy link
Contributor

@JakobJingleheimer JakobJingleheimer commented May 17, 2022

When a custom loader is used to load a remote asset via http(s), the internal cache is circumvented. This results in a nested import's resolution to run into an unsettled promise (causing parentURL to be undefined instead of its actual value).

fixes #42721

TODO:

  • Add test case(s) for this specific scenario (the current test cases do not cover nested imports).

@JakobJingleheimer JakobJingleheimer added the loaders Issues and PRs related to ES module loaders label May 17, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 17, 2022

Review requested:

  • @nodejs/loaders
  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels May 17, 2022
@JakobJingleheimer JakobJingleheimer marked this pull request as ready for review May 20, 2022 16:18
@JakobJingleheimer
Copy link
Contributor Author

JakobJingleheimer commented May 20, 2022

@guybedford I quickly looked into the ModuleJob avenue you suggested, but I'm pretty sure it will take longer than the next ~hour I have before being gone a week. To resolve this bug sooner rather than later, I say we go with this workable-but-not-quite-ideal fix and improve it in a follow-up (which I'd be happy to do when I get back, probably after switching resolve to synchronous?). I added an item to the Loaders project for it so we don't forget.

I updated the jsdocs and added some "there be dragons" code docs.

Copy link
Member

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

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

Please confirm that the tests fail without your lib/ changes.

@JakobJingleheimer
Copy link
Contributor Author

I did (see #43130 (comment))

@guybedford
Copy link
Contributor

guybedford commented May 20, 2022

@JakobJingleheimer I hope you don't mind I pushed a commit to add a fetch cache guard to avoid double fetching and clarify the contract.

As mentioned before the architectural fix should be:

  • Allow the load hook to return a { responseUrl } as separate to the initial resolved URL, which is stored on the module job.
  • Then instead of reading off from the fetch cache, rather read off from the stored module job responseUrl instead of this function (and then, is a fetch cache even needed?)

For now it would be great to get this fix merged certainly.

guybedford added a commit to guybedford/node that referenced this pull request May 21, 2022
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

A few optional nits, otherwise LGTM :)

test/fixtures/es-module-loaders/http-loader.mjs Outdated Show resolved Hide resolved
test/fixtures/es-module-loaders/http-loader.mjs Outdated Show resolved Hide resolved
test/fixtures/es-module-loaders/http-loader.mjs Outdated Show resolved Hide resolved
test/es-module/test-esm-loader-http-imports.mjs Outdated Show resolved Hide resolved
@lmichailian
Copy link

lmichailian commented May 23, 2022

Hi! When do you merge this PR or how can I help with this? thanks!

PD: There is some workaround ?

@JakobJingleheimer
Copy link
Contributor Author

JakobJingleheimer commented May 24, 2022

@guybedford I don't mind 🙂 thanks! Good improvement!

@aduh95 good nits, especially the test one. I can't apply them right now. Would you mind batch-committing them and add the label to merge? (GitHub mobile doesn't support it)

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels May 24, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 24, 2022
@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label May 25, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 25, 2022
@nodejs-github-bot nodejs-github-bot merged commit b5ed1bd into nodejs:master May 25, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in b5ed1bd

@JakobJingleheimer JakobJingleheimer deleted the esm/fix-http-import-via-custom-loader branch May 29, 2022 08:16
@bengl
Copy link
Member

bengl commented May 30, 2022

Hi @JakobJingleheimer. This doesn't land cleanly on v18.x. Could you submit a backport PR?

@bengl bengl added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label May 30, 2022
@targos
Copy link
Member

targos commented May 30, 2022

I am not on my computer to link to it, but this depends on a previous loaders PR that has the backport blocked label

@JakobJingleheimer
Copy link
Contributor Author

What @targos said (it's not supposed to yet). Sorry, should I apply a label or something?

@bengl
Copy link
Member

bengl commented May 30, 2022

@JakobJingleheimer I'll just change the label to backport blocked. If someone can throw the link in here to the previous PR once they find it, that would be great, so we can make sure that this one lands on a release when that one does.

@bengl bengl added backport-blocked-v18.x PRs that should land on the v18.x-staging branch but are blocked by another PR's pending backport. and removed backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. labels May 30, 2022
@JakobJingleheimer
Copy link
Contributor Author

#42623

danielleadams pushed a commit that referenced this pull request Jun 11, 2022
PR-URL: #43130
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@danielleadams danielleadams mentioned this pull request Jun 11, 2022
@JakobJingleheimer JakobJingleheimer removed the backport-blocked-v18.x PRs that should land on the v18.x-staging branch but are blocked by another PR's pending backport. label Jun 18, 2022
targos pushed a commit that referenced this pull request Jul 12, 2022
PR-URL: #43130
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jul 19, 2022
PR-URL: #43130
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
PR-URL: #43130
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
PR-URL: nodejs/node#43130
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

custom loader fails to resolve nested http(s) import
8 participants