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

[Fix] babel's "loose mode" class transform enbrittles BufferList #428

Merged
merged 1 commit into from Feb 13, 2020

Conversation

@ljharb
Copy link
Member

ljharb commented Feb 13, 2020

Object.defineProperty(Object.prototype, util.inspect.custom, {
  set(v) { throw 'this should not happen inside readable-stream'; }
});

With this change, I believe the output will use [[Define]] instead of [[Set]] for https://github.com/nodejs/node/blob/c101251a95cc82142bee4637f8db6cc360a06d82/lib/internal/streams/buffer_list.js#L167, and thus no longer fail when Object.prototype is modified.

```js
Object.defineProperty(Object.prototype, util.inspect.custom, {
  set(v) { throw 'this should not happen inside readable-stream'; }
});
```

With this change, I believe the output will use [[Define]] instead of [[Set]] for https://github.com/nodejs/node/blob/c101251a95cc82142bee4637f8db6cc360a06d82/lib/internal/streams/buffer_list.js#L167, and thus no longer fail when Object.prototype is modified.
@ljharb ljharb requested a review from mcollina Feb 13, 2020
@ljharb

This comment has been minimized.

Copy link
Member Author

ljharb commented Feb 13, 2020

Seems like vweevers@432d4b7 is when it first was put into loose mode, in #299, due to #293.

Does readable-stream still support pre-ES5 environments? #344 implies that it does not.

@mcollina

This comment has been minimized.

Copy link
Member

mcollina commented Feb 13, 2020

We support IE11 in readable-stream v3, while we still support pre-es5 environments in v2. This can land but not be backported.

@mcollina

This comment has been minimized.

Copy link
Member

mcollina commented Feb 13, 2020

Can you also regenerate the code?

cd build
node build.js v10.19.0
@ljharb

This comment has been minimized.

Copy link
Member Author

ljharb commented Feb 13, 2020

just v3 is fine; I’m mainly concerned with the version of this that’s vendored in core; that one doesn’t need to run Babel at all :-)

will update the build shortly

@ljharb ljharb force-pushed the ljharb-babel-spec-mode branch from 744c5d2 to 186503e Feb 13, 2020
.babelrc Show resolved Hide resolved
Copy link
Member

mcollina left a comment

lgtm

@mcollina

This comment has been minimized.

Copy link
Member

mcollina commented Feb 13, 2020

cc @vweevers what do you think?

@vweevers

This comment has been minimized.

Copy link
Contributor

vweevers commented Feb 13, 2020

👍

@mcollina mcollina merged commit 3bbf2d6 into master Feb 13, 2020
2 checks passed
2 checks passed
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
@mcollina mcollina deleted the ljharb-babel-spec-mode branch Feb 13, 2020
@ljharb

This comment has been minimized.

Copy link
Member Author

ljharb commented Feb 13, 2020

yay! what are the next steps to get this into core?

@mcollina

This comment has been minimized.

Copy link
Member

mcollina commented Feb 13, 2020

I think you'll have to wait for npm to do a release, and then update core to that. Or you can force a replacement directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.