-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: mark importAssertions
as required
#46164
esm: mark importAssertions
as required
#46164
Conversation
We already always specify a value, and failing to do so would likely be a bug.
Review requested:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure neglecting to provide a value is a bug, but happy to be corrected. Could you provide some rationale as to how/why it might be?
V8 always provides an object that represents the import assertions for a specific import, so if the value is ever |
Ah, maybe yes 🤔
I don't know of a specific scenario; I think likely never. The optional param is a convenience. |
In this case, I think it's more of a footgun than a convenience. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed in #46153 (review)
Commit Queue failed- Loading data for nodejs/node/pull/46164 ✔ Done loading data for nodejs/node/pull/46164 ----------------------------------- PR info ------------------------------------ Title esm: mark `importAssertions` as required (#46164) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch aduh95:import-assertions-required -> nodejs:main Labels esm, author ready, needs-ci Commits 4 - esm: mark `importAssertions` as required - Apply suggestions from code review - Update lib/internal/modules/esm/loader.js - Update lib/internal/modules/esm/loader.js Committers 2 - Antoine du Hamel - GitHub PR-URL: https://github.com/nodejs/node/pull/46164 Reviewed-By: Geoffrey Booth Reviewed-By: Michaël Zasso ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/46164 Reviewed-By: Geoffrey Booth Reviewed-By: Michaël Zasso -------------------------------------------------------------------------------- ℹ This PR was created on Tue, 10 Jan 2023 20:52:50 GMT ✔ Approvals: 2 ✔ - Geoffrey Booth (@GeoffreyBooth) (TSC): https://github.com/nodejs/node/pull/46164#pullrequestreview-1243081831 ✔ - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/46164#pullrequestreview-1243560207 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2023-01-12T12:56:16Z: https://ci.nodejs.org/job/node-test-pull-request/48954/ - Querying data for job/node-test-pull-request/48954/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 46164 From https://github.com/nodejs/node * branch refs/pull/46164/merge -> FETCH_HEAD ✔ Fetched commits as 91ca2d404192..cd7f6e652d81 -------------------------------------------------------------------------------- Auto-merging lib/internal/modules/esm/loader.js [main 26c2ef0f51] esm: mark `importAssertions` as required Author: Antoine du Hamel Date: Tue Jan 10 21:47:00 2023 +0100 1 file changed, 4 insertions(+), 12 deletions(-) Auto-merging lib/internal/modules/esm/loader.js [main f85caff68f] Apply suggestions from code review Author: Antoine du Hamel Date: Tue Jan 10 23:26:57 2023 +0100 1 file changed, 6 insertions(+), 2 deletions(-) Auto-merging lib/internal/modules/esm/loader.js [main fac5d98694] Update lib/internal/modules/esm/loader.js Author: Antoine du Hamel Date: Tue Jan 10 23:31:30 2023 +0100 1 file changed, 3 insertions(+), 3 deletions(-) Auto-merging lib/internal/modules/esm/loader.js [main 41bb707d5d] Update lib/internal/modules/esm/loader.js Author: Antoine du Hamel Date: Tue Jan 10 23:31:51 2023 +0100 1 file changed, 1 insertion(+), 1 deletion(-) ✔ Patches applied There are 4 commits in the PR. Attempting autorebase. Rebasing (2/8)https://github.com/nodejs/node/actions/runs/3905995776 |
Landed in 31ea7be |
We already always specify a value, and failing to do so would likely be a bug. PR-URL: nodejs#46164 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
We already always specify a value, and failing to do so would likely be a bug. PR-URL: #46164 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
This comment was marked as outdated.
This comment was marked as outdated.
We already always specify a value, and failing to do so would likely be a bug. PR-URL: #46164 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
We already always specify a value, and failing to do so would likely be a bug. PR-URL: #46164 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
We already always specify a value, and failing to do so would likely be a bug.