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: allow resolve to optionally return import assertions #46153

Conversation

GeoffreyBooth
Copy link
Member

As part of #44710 @targos discovered that our test fixture loader for “assertionless” JSON imports—import data from './file.json' where there’s no assert { type: 'json' }—was only working because it mutated the context.importAssertions object, which doesn’t work on the “loaders off-thread” branch. But it arguably shouldn’t work on main either, as mutating function input is a poor way for a hook to return information.

This PR adds an optional importAssertions property to the object returned by the resolve hook, to enable the use case where a module is cached with different import assertions than were present in the original source. I think this is the “correct” way that user hooks should inform the internal module loader what assertion type to use as part of the module cache key, rather than relying on input mutation.

cc @nodejs/loaders

@GeoffreyBooth GeoffreyBooth added esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders labels Jan 10, 2023
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jan 10, 2023
@GeoffreyBooth GeoffreyBooth force-pushed the resolve-hook-returns-import-assertions branch from 52f571c to c797d07 Compare January 10, 2023 01:32
@GeoffreyBooth GeoffreyBooth requested a review from targos January 10, 2023 01:32
@GeoffreyBooth GeoffreyBooth force-pushed the resolve-hook-returns-import-assertions branch 2 times, most recently from d34bf01 to e80eaa6 Compare January 10, 2023 01:55
@nodejs-github-bot
Copy link
Collaborator

@GeoffreyBooth GeoffreyBooth removed the needs-ci PRs that need a full CI run. label Jan 10, 2023
@GeoffreyBooth GeoffreyBooth force-pushed the resolve-hook-returns-import-assertions branch from e80eaa6 to 1a9e7d6 Compare January 10, 2023 06:27
@nodejs-github-bot
Copy link
Collaborator

doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated
Comment on lines 784 to 788
Import assertions are part of the cache key for saving loaded modules into the
Node.js internal module cache. The `resolve` hook is responsible for returning
an `importAssertions` object if the module should be cached with different
assertions than were present in the source code (for example, if no assertions
were present but the module should be cached with assertions
`{ type: 'json' }`).
Copy link
Member

Choose a reason for hiding this comment

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

I feel like the key thing is an implementation detail. The reason a loader needs to customize import assertions is to support modules that import JSON without the assertion (bypass the validation).

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’m not sure what you want changed, if anything?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know how to write it differently. If you think users can understand the current wording that's fine.

doc/api/esm.md Outdated Show resolved Hide resolved
lib/internal/modules/esm/hooks.js Outdated Show resolved Hide resolved
lib/internal/modules/esm/hooks.js Outdated Show resolved Hide resolved
lib/internal/modules/esm/loader.js Outdated Show resolved Hide resolved
lib/internal/modules/esm/loader.js Outdated Show resolved Hide resolved
lib/internal/modules/esm/resolve.js Outdated Show resolved Hide resolved
lib/internal/modules/esm/resolve.js Outdated Show resolved Hide resolved
lib/internal/modules/esm/resolve.js Outdated Show resolved Hide resolved
lib/internal/modules/esm/resolve.js Outdated Show resolved Hide resolved
@aduh95 aduh95 changed the title esm: resolve optionally returns import assertions esm: allow resolve to optionally return import assertions Jan 10, 2023
@aduh95
Copy link
Contributor

aduh95 commented Jan 10, 2023

The first word of the commit message should be an imperative verb:

* be prefixed with the name of the changed [subsystem](#appendix-subsystems)
and start with an imperative verb. Check the output of `git log --oneline

resolve is an imperative verb, but is not used as one here and it's quite confusing.

@GeoffreyBooth GeoffreyBooth force-pushed the resolve-hook-returns-import-assertions branch from 1a9e7d6 to 0a01441 Compare January 10, 2023 18:28
lib/internal/modules/esm/loader.js Outdated Show resolved Hide resolved
lib/internal/modules/esm/loader.js Outdated Show resolved Hide resolved
lib/internal/modules/esm/loader.js Outdated Show resolved Hide resolved
@GeoffreyBooth GeoffreyBooth force-pushed the resolve-hook-returns-import-assertions branch from 0a01441 to 246694f Compare January 10, 2023 19:32
@GeoffreyBooth GeoffreyBooth force-pushed the resolve-hook-returns-import-assertions branch from 246694f to c3a2e8b Compare January 10, 2023 20:04
@GeoffreyBooth GeoffreyBooth force-pushed the resolve-hook-returns-import-assertions branch 2 times, most recently from e9822be to 349d054 Compare January 10, 2023 20:18
@bmeck
Copy link
Member

bmeck commented Jan 11, 2023 via email

@GeoffreyBooth
Copy link
Member Author

Because resolution is explicitly banned from using import assertions to affect it in spec: import assertions (tc39.es)

Yes, but that wouldn’t apply to user loaders, which are allowed to violate spec. The reference @aduh95 found is in a user loader test fixture.

@ljharb

This comment was marked as off-topic.

@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 12, 2023
@nodejs-github-bot nodejs-github-bot merged commit 91ca2d4 into nodejs:main Jan 12, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 91ca2d4

@RafaelGSS
Copy link
Member

Hi @GeoffreyBooth! This didn't land cleanly on v19.x-staging, can you create a manual backport? Thanks

@GeoffreyBooth
Copy link
Member Author

GeoffreyBooth commented Jan 17, 2023

@RafaelGSS This needs to wait on #45869 (comment).

@juanarbol juanarbol added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Jan 25, 2023
@juanarbol
Copy link
Member

Hey, this is not landing cleanly on v18.x line :(

@GeoffreyBooth
Copy link
Member Author

Hey, this is not landing cleanly on v18.x line :(

What if you land #43772 and then #45869? Hopefully this can land cleanly after those two go first?

@targos
Copy link
Member

targos commented Mar 14, 2023

Lands cleanly indeed.

targos pushed a commit that referenced this pull request Mar 14, 2023
PR-URL: #46153
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
@ljharb
Copy link
Member

ljharb commented Mar 14, 2023

Given the tenuous status of import assertions, should this change be backported?

targos pushed a commit that referenced this pull request Nov 10, 2023
PR-URL: #46153
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#46153
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#46153
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
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. backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants