GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — esm: fallback to `readFileSync` when `source` is nullish by aduh95 · Pull Request #50825 · nodejs/node

Conversation

@aduh95

@aduh95 aduh95 commented Nov 20, 2023

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji lizthegrey reacted with hooray emoji
All reactions
  • 👍 1 reaction
  • 🎉 1 reaction
@nodejs-github-bot

nodejs-github-bot commented Nov 20, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/loaders
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
@aduh95
esm: fallback to getSource when load returns nullish source
e1f1a80 
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.
@aduh95
aduh95 force-pushed the cjs-nullish-source branch from 594830d to e1f1a80 Compare November 20, 2023 16:48
GeoffreyBooth
GeoffreyBooth approved these changes Nov 20, 2023
View reviewed changes

@GeoffreyBooth GeoffreyBooth left a comment

Copy link
Copy Markdown
Member

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 think this avoids the source being read from disk twice?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@aduh95

aduh95 commented Nov 20, 2023

Copy link
Copy Markdown
Contributor Author

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 😅
JakobJingleheimer reacted with laugh emoji
All reactions
  • 😄 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95 aduh95 added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Nov 20, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 20, 2023
@nodejs-github-bot

nodejs-github-bot commented Nov 20, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/55794/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95 aduh95 mentioned this pull request Nov 21, 2023
esmocha broken by ESM hook changes? nodejs/citgm#1029
Closed
@targos

targos commented Nov 21, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Small CITGM run to see if nodejs/citgm#1029 is fixed: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3347/

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
anonrig
anonrig reviewed Nov 21, 2023
View reviewed changes
Comment thread lib/internal/modules/esm/translators.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
anonrig
anonrig reviewed Nov 21, 2023
View reviewed changes
Comment thread lib/internal/modules/esm/translators.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@aduh95
fixup! esm: fallback to getSource when load returns nullish source
11c610a
@merceyz

merceyz commented Nov 21, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 23, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 23, 2023
@nodejs-github-bot

nodejs-github-bot commented Nov 23, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/55882/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aduh95
aduh95 commented Nov 25, 2023
View reviewed changes
Comment thread test/es-module/test-esm-loader-hooks.mjs Resolved

Uh oh!

There was an error while loading. Please reload this page.
aduh95
aduh95 commented Nov 25, 2023
View reviewed changes
Comment thread test/es-module/test-esm-loader-hooks.mjs Resolved

Uh oh!

There was an error while loading. Please reload this page.
@aduh95
Apply suggestions from code review
b58772c
@nodejs-github-bot

nodejs-github-bot commented Nov 25, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/55919/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
JakobJingleheimer
JakobJingleheimer approved these changes Nov 27, 2023
View reviewed changes

@JakobJingleheimer JakobJingleheimer left a comment

Copy link
Copy Markdown
Member

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.

🙌

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@aduh95
remove weird condition
a359b02
@nodejs-github-bot

nodejs-github-bot commented Nov 27, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/55961/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95
fixup! remove weird condition
3cde424
@nodejs-github-bot

nodejs-github-bot commented Nov 28, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/55976/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Nov 28, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/55986/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95
aduh95 merged commit 4eafcf8 into nodejs:main Nov 29, 2023
@aduh95

aduh95 commented Nov 29, 2023

Copy link
Copy Markdown
Contributor Author

Landed in 4eafcf8
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
@aduh95 @RafaelGSS
esm: fallback to getSource when load returns nullish source
50f336c 
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
v21.3.0 proposal #50954
Merged
@lizthegrey

lizthegrey commented Dec 8, 2023

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023
@aduh95 @UlisesGascon
esm: fallback to getSource when load returns nullish source
8df32ab 
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
v20.11.0 proposal #51124
Merged
UlisesGascon pushed a commit that referenced this pull request Dec 13, 2023
@aduh95 @UlisesGascon
esm: fallback to getSource when load returns nullish source
fbdf291 
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

lizthegrey commented Dec 14, 2023

Copy link
Copy Markdown

Thanks for the backport.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
UlisesGascon pushed a commit that referenced this pull request Dec 15, 2023
@aduh95 @UlisesGascon
esm: fallback to getSource when load returns nullish source
06cee79 
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
@aduh95 @UlisesGascon
esm: fallback to getSource when load returns nullish source
843d5f8 
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 label Dec 24, 2023
codebytere added a commit to electron/electron that referenced this pull request Jan 10, 2024
@codebytere
esm: fallback to readFileSync when source is nullish
d21d3d0 
nodejs/node#50825
zcbenz pushed a commit to electron/electron that referenced this pull request Jan 12, 2024
@codebytere @zcbenz
esm: fallback to readFileSync when source is nullish
acb8b30 
nodejs/node#50825
zcbenz pushed a commit to electron/electron that referenced this pull request Jan 12, 2024
@codebytere @zcbenz
esm: fallback to readFileSync when source is nullish
3b334f8 
nodejs/node#50825
codebytere added a commit to electron/electron that referenced this pull request Jan 15, 2024
@codebytere
esm: fallback to readFileSync when source is nullish
38c94c5 
nodejs/node#50825
codebytere added a commit to electron/electron that referenced this pull request Jan 15, 2024
@codebytere
esm: fallback to readFileSync when source is nullish
7928388 
nodejs/node#50825
codebytere added a commit to electron/electron that referenced this pull request Jan 16, 2024
@codebytere
esm: fallback to readFileSync when source is nullish
0c5dbcb 
nodejs/node#50825
codebytere added a commit to electron/electron that referenced this pull request Jan 18, 2024
@codebytere
esm: fallback to readFileSync when source is nullish
67152ba 
nodejs/node#50825
jkleinsc pushed a commit to electron/electron that referenced this pull request Jan 18, 2024
@codebytere @jkleinsc
esm: fallback to readFileSync when source is nullish
1f2d223 
nodejs/node#50825
codebytere added a commit to electron/electron that referenced this pull request Jan 18, 2024
@codebytere
esm: fallback to readFileSync when source is nullish
e604a2d 
nodejs/node#50825
jkleinsc pushed a commit to electron/electron that referenced this pull request Jan 18, 2024
@electron-roller @codebytere @zcbenz
chore: bump node to v20.11.0 (main) (#40941)
f4ee3c1 
* 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 label Nov 2, 2024
@privatenumber privatenumber mentioned this pull request Aug 7, 2026
[yarn] [ERR_INVALID_RETURN_PROPERTY_VALUE]: Expected array buffer, or typed array to be returned for the "source" from the "transformSource" function but got undefined. privatenumber/tsx#338
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anonrig anonrig anonrig left review comments
@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes
@JakobJingleheimer JakobJingleheimer JakobJingleheimer approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ERR_INVALID_RETURN_PROPERTY_VALUE with load hook and commonjs

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@aduh95 @nodejs-github-bot @targos @merceyz @lizthegrey @GeoffreyBooth @anonrig @JakobJingleheimer
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.