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.

Sign up for GitHub

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

Jump to bottom

module: fix initial ModuleWrap with non-string source #33443

Closed
wants to merge 1 commit into from
Closed

module: fix initial ModuleWrap with non-string source #33443

wants to merge 1 commit into from

Conversation

himself65
Copy link
Member

@himself65 himself65 commented May 17, 2020 •
edited
Loading

Fixes #33441

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the esm Issues and PRs related to the ECMAScript Modules implementation. label May 17, 2020
@himself65 himself65 force-pushed the 20200517-fix branch 3 times, most recently from acbe4ff to 793b36f Compare May 17, 2020 05:07
@guybedford
Copy link
Contributor

Note that #32202 fixes this and in a way that will still throw for non-buffer or string values so would be slightly stricter.

@bmeck did you still want to merge #32202? The CI failure there seems like it's just an assertion that needs fixing to me.

@himself65
Copy link
Member Author

note that expected behavior should throw a JS error

@himself65 himself65 marked this pull request as ready for review May 17, 2020 06:09
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/31364/

@bmeck
Copy link
Member

bmeck commented May 17, 2020 via email

@himself65 himself65 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 17, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/31368/

@bmeck
Copy link
Member

bmeck commented May 18, 2020

I've rebased my PR, @himself65 would it be ok to merge my PR, it has slightly different semantics since it won't stringify every kind of object that makes it to ModuleWrap. That behavior is slightly different from this PR and I'm curious if you had a use case for values that are not Buffers or TypedArrays being passed as source text before I move forward with my PR.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 20, 2020 •
edited by himself65
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/31444/ (✅)

@guybedford
Copy link
Contributor

I would prefer if we can land @bmeck's PR as well since it reserves API space while handling and documenting edge cases.

@himself65
Copy link
Member Author

#32202 fixed this, closing

@himself65 himself65 closed this May 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Crash in module_wrap.cc with malformed loader
6 participants
@himself65 @guybedford @nodejs-github-bot @bmeck @addaleax @cjihrig