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: fallback to readFileSync when source is nullish #50825

Merged
merged 5 commits into from
Nov 29, 2023

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Nov 20, 2023

When using the Modules Customization Hooks API to load CommonJS modules, we want to support the returned value of defaultLoad which must be nullish to preserve backward compatibility. This can be achieved by fetching the source from the translator.

Fixes: #50435

@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 Nov 20, 2023
When using the Modules Customization Hooks API to load CommonJS modules,
we want to support the returned value of `defaultLoad` which must be
nullish to preserve backward compatibility. This can be achieved by
fetching the source from the translator.
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.

I think this avoids the source being read from disk twice?

@aduh95
Copy link
Contributor Author

aduh95 commented Nov 20, 2023

I think this avoids the source being read from disk twice?

Nope, this is a different issue. It only avoids the source being read 0 times from disk 😅

@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. labels Nov 20, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 20, 2023
@nodejs-github-bot
Copy link
Collaborator

@targos
Copy link
Member

targos commented Nov 21, 2023

@merceyz
Copy link
Member

merceyz commented Nov 21, 2023

Edit: result will be in https://ci.nodejs.org/job/citgm-smoker/nodes=fedora-latest-x64/3347/

@targos could you run another CITGM test with nodejs/citgm#1022 included?
nodejs/citgm#1022 enables testing the Yarn ESM loader which might be affected by this change.

@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 23, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 23, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Copy link
Contributor

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

🙌

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 merged commit 4eafcf8 into nodejs:main Nov 29, 2023
47 checks passed
@aduh95
Copy link
Contributor Author

aduh95 commented Nov 29, 2023

Landed in 4eafcf8

@aduh95 aduh95 deleted the cjs-nullish-source branch November 29, 2023 09:12
RafaelGSS pushed a commit that referenced this pull request Nov 29, 2023
When using the Modules Customization Hooks API to load CommonJS modules,
we want to support the returned value of `defaultLoad` which must be
nullish to preserve backward compatibility. This can be achieved by
fetching the source from the translator.

PR-URL: #50825
Fixes: #50435
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Nov 29, 2023
@lizthegrey
Copy link

Can we get this tagged as to backport to 20.x release series? Thanks.

UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023
When using the Modules Customization Hooks API to load CommonJS modules,
we want to support the returned value of `defaultLoad` which must be
nullish to preserve backward compatibility. This can be achieved by
fetching the source from the translator.

PR-URL: #50825
Fixes: #50435
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
@UlisesGascon UlisesGascon mentioned this pull request Dec 12, 2023
UlisesGascon pushed a commit that referenced this pull request Dec 13, 2023
When using the Modules Customization Hooks API to load CommonJS modules,
we want to support the returned value of `defaultLoad` which must be
nullish to preserve backward compatibility. This can be achieved by
fetching the source from the translator.

PR-URL: #50825
Fixes: #50435
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
@lizthegrey
Copy link

Thanks for the backport.

UlisesGascon pushed a commit that referenced this pull request Dec 15, 2023
When using the Modules Customization Hooks API to load CommonJS modules,
we want to support the returned value of `defaultLoad` which must be
nullish to preserve backward compatibility. This can be achieved by
fetching the source from the translator.

PR-URL: #50825
Fixes: #50435
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Dec 19, 2023
When using the Modules Customization Hooks API to load CommonJS modules,
we want to support the returned value of `defaultLoad` which must be
nullish to preserve backward compatibility. This can be achieved by
fetching the source from the translator.

PR-URL: #50825
Fixes: #50435
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
@aduh95 aduh95 added the lts-watch-v18.x PRs that may need to be released in v18.x. label Dec 24, 2023
codebytere added a commit to electron/electron that referenced this pull request Jan 10, 2024
zcbenz pushed a commit to electron/electron that referenced this pull request Jan 12, 2024
zcbenz pushed a commit to electron/electron that referenced this pull request Jan 12, 2024
codebytere added a commit to electron/electron that referenced this pull request Jan 15, 2024
codebytere added a commit to electron/electron that referenced this pull request Jan 15, 2024
codebytere added a commit to electron/electron that referenced this pull request Jan 16, 2024
codebytere added a commit to electron/electron that referenced this pull request Jan 18, 2024
jkleinsc pushed a commit to electron/electron that referenced this pull request Jan 18, 2024
codebytere added a commit to electron/electron that referenced this pull request Jan 18, 2024
jkleinsc pushed a commit to electron/electron that referenced this pull request Jan 18, 2024
* chore: bump node in DEPS to v20.11.0

* module: bootstrap module loaders in shadow realm

nodejs/node#48655

* src: add commit hash shorthand in zlib version

nodejs/node#50158

* v8,tools: expose necessary V8 defines

nodejs/node#50820

* esm: do not call getSource when format is commonjs

nodejs/node#50465

* esm: fallback to readFileSync when source is nullish

nodejs/node#50825

* vm: allow dynamic import with a referrer realm

nodejs/node#50360

* test: skip test-diagnostics-channel-memory-leak.js

nodejs/node#50327

* esm: do not call getSource when format is commonjs

nodejs/node#50465

* lib: fix assert throwing different error messages in ESM and CJS

nodejs/node#50634

* src: fix compatility with upcoming V8 12.1 APIs

nodejs/node#50709

* deps: update base64 to 0.5.1

nodejs/node#50629

* src: avoid silent coercion to signed/unsigned int

nodejs/node#50663

* src: fix compatility with upcoming V8 12.1 APIs

nodejs/node#50709

* chore: fix patch indices

* chore: update patches

* test: disable TLS cipher test

This can't be enabled owing to BoringSSL incompatibilities.

nodejs/node#50186

* fix: check for Buffer and global definition in shadow realm

nodejs/node#51239

* test: disable parallel/test-shadow-realm-custom-loader

Incompatible with our asar logic, resulting in the following failure:

> Failed to CompileAndCall electron script: electron/js2c/asar_bundle

* chore: remove deleted parallel/test-crypto-modp1-error test

* test: make test-node-output-v8-warning generic

nodejs/node#50421

* chore: fixup ModuleWrap patch

* test: match wpt/streams/transferable/transform-stream-members.any.js to upstream

* fix: sandbox is not enabled on arm

* chore: disable v8 sandbox on ia32/arm

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <[email protected]>
Co-authored-by: Cheng Zhao <[email protected]>
@aduh95 aduh95 removed the lts-watch-v18.x PRs that may need to be released in v18.x. label Nov 2, 2024
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. 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.

ERR_INVALID_RETURN_PROPERTY_VALUE with load hook and commonjs
8 participants