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 inconsistency with importAssertion in resolve hook #55365

Merged
merged 2 commits into from
Oct 15, 2024

Conversation

yesmeck
Copy link
Contributor

@yesmeck yesmeck commented Oct 12, 2024

As the documentation says, the context.importAssertions should be still supported and emit a warning. This is true for the load hook, but not correct for context of the resolve hook.

This PR is tring to fix the inconsistency.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@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 Oct 12, 2024
@yesmeck yesmeck marked this pull request as ready for review October 12, 2024 11:09
@yesmeck yesmeck force-pushed the alias-import-assertion-for-resolve-hook branch from 8530577 to b923ecb Compare October 12, 2024 11:11
@yesmeck yesmeck force-pushed the alias-import-assertion-for-resolve-hook branch from b923ecb to 213c6eb Compare October 13, 2024 10:49
@yesmeck
Copy link
Contributor Author

yesmeck commented Oct 13, 2024

Updated the commit message and fixed the lint issue.

@yesmeck yesmeck force-pushed the alias-import-assertion-for-resolve-hook branch from 213c6eb to 0a3ceda Compare October 13, 2024 11:33
@yesmeck
Copy link
Contributor Author

yesmeck commented Oct 13, 2024

Fixed test/es-module/test-esm-loader-hooks.mjs.

As the [documentation](https://nodejs.org/docs/latest/api/module.html#customization-hooks:~:text=The%20property%20context.importAssertions%20is%20replaced%20with%20context.importAttributes.%20Using%20the%20old%20name%20is%20still%20supported%20and%20will%20emit%20an%20experimental%20warning.) says, the `context.importAssertion` should be still supported and emit a warning. This is true for the load hook, but not correct for context of the resolve hook.

This PR is tring to fix the inconsistency.
@yesmeck yesmeck force-pushed the alias-import-assertion-for-resolve-hook branch from 0a3ceda to 78cfeef Compare October 13, 2024 11:39
@yesmeck
Copy link
Contributor Author

yesmeck commented Oct 13, 2024

Made a minor change to the test of the load hook to make it consistent with the test of the resolve hook.

Copy link

codecov bot commented Oct 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.41%. Comparing base (f98d9c1) to head (5bb8741).
Report is 37 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55365      +/-   ##
==========================================
+ Coverage   88.40%   88.41%   +0.01%     
==========================================
  Files         652      652              
  Lines      186777   186889     +112     
  Branches    36029    36067      +38     
==========================================
+ Hits       165114   165243     +129     
+ Misses      14919    14886      -33     
- Partials     6744     6760      +16     
Files with missing lines Coverage Δ
lib/internal/modules/esm/hooks.js 90.68% <100.00%> (+0.26%) ⬆️

... and 58 files with indirect coverage changes

@yesmeck yesmeck changed the title esm: fix inconsistency of importAssertion between resolve and load hook esm: fix inconsistency of importAssertions between resolve and load hook Oct 13, 2024
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 15, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 15, 2024
@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 merged commit c0aebed into nodejs:main Oct 15, 2024
21 checks passed
@aduh95
Copy link
Contributor

aduh95 commented Oct 15, 2024

Landed in c0aebed

@yesmeck yesmeck changed the title esm: fix inconsistency of importAssertions between resolve and load hook esm: fix inconsistency with importAssertion in resolve hook Oct 16, 2024
aduh95 pushed a commit that referenced this pull request Oct 19, 2024
As the documentation states, the `context.importAssertion` should be
still supported and emit a warning. This is true for the `load` hook,
but not correct for context of the `resolve` hook.

This commit fixes the inconsistency.

PR-URL: #55365
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@aduh95 aduh95 mentioned this pull request Oct 24, 2024
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
As the documentation states, the `context.importAssertion` should be
still supported and emit a warning. This is true for the `load` hook,
but not correct for context of the `resolve` hook.

This commit fixes the inconsistency.

PR-URL: nodejs#55365
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
As the documentation states, the `context.importAssertion` should be
still supported and emit a warning. This is true for the `load` hook,
but not correct for context of the `resolve` hook.

This commit fixes the inconsistency.

PR-URL: nodejs#55365
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants