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

util: fix deprecated class prototype #8105

Closed
wants to merge 1 commit into from

Conversation

bengl
Copy link
Member

@bengl bengl commented Aug 14, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

util

Description of change

Ensure the wrapped class prototype is exactly the unwrapped class
prototype, rather than an object whose prototype is the unwrapped
class prototype.

This ensures that instances of the unwrapped class are instances
of the wrapped class. This is useful when both a wrapped class and
a factory for the unwrapped class are both exposed.

Ref: #8103

Ensure the wrapped class prototype is exactly the unwrapped class
prototype, rather than an object whose prototype is the unwrapped
class prototype.

This ensures that instances of the unwrapped class are instances
of the wrapped class. This is useful when both a wrapped class and
a factory for the unwrapped class are both exposed.

Ref: nodejs#8103
@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Aug 14, 2016
@addaleax
Copy link
Member

LGTM if CI is happy: https://ci.nodejs.org/job/node-test-commit/4570/

@evanlucas
Copy link
Contributor

LGTM. Thoughts on whether this should land in v6?

@Fishrock123
Copy link
Contributor

seems good for v6/v4 to me

@jasnell
Copy link
Member

jasnell commented Aug 15, 2016

LGTM

jasnell pushed a commit that referenced this pull request Aug 18, 2016
Ensure the wrapped class prototype is exactly the unwrapped class
prototype, rather than an object whose prototype is the unwrapped
class prototype.

This ensures that instances of the unwrapped class are instances
of the wrapped class. This is useful when both a wrapped class and
a factory for the unwrapped class are both exposed.

Ref: #8103
PR-URL: #8105
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Aug 18, 2016

Landed in 249bb8d

@jasnell jasnell closed this Aug 18, 2016
evanlucas pushed a commit that referenced this pull request Aug 20, 2016
Ensure the wrapped class prototype is exactly the unwrapped class
prototype, rather than an object whose prototype is the unwrapped
class prototype.

This ensures that instances of the unwrapped class are instances
of the wrapped class. This is useful when both a wrapped class and
a factory for the unwrapped class are both exposed.

Ref: #8103
PR-URL: #8105
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

@bengl would you be willing to backport to v4?

@bengl
Copy link
Member Author

bengl commented Sep 30, 2016

Sure. That just means a new PR against v4.x-staging with this commit (preserving review metadata), rebased, etc., right?

Are there docs on backporting procedure?

@addaleax
Copy link
Member

@bengl Yes, sounds about right to me. What I’m usually doing is branching off v4.x-staging and cherry-picking the commit that landed in master, then resolving any conflicts.

Are there docs on backporting procedure?

I don’t think so but it might be helpful, yeah.

@MylesBorins
Copy link
Contributor

A backporting doc is a really good idea.

@bengl
Copy link
Member Author

bengl commented Sep 30, 2016

@thealphanerd this PR/commit depends on 320f433 which uses Reflect.construct, which isn't available in v4.x. Without Reflect.construct, I'm not sure deprecated classes can be done correctly anyway, so having the correct prototype is kind of moot.

Unless there's a way of re-implementing 320f433 without using Reclect.construct, I'd say this probably isn't backportable to v4.x.

@MylesBorins
Copy link
Contributor

switching to don't land for now. Added note to other PR thread. Thanks @bengl

@vdeturckheim
Copy link
Member

vdeturckheim commented Sep 30, 2016

@bengl @thealphanerd
I believe something like
new (Function.prototype.bind.apply(fn, [null].concat(Array.from(arguments))))() should work to replace Reflect.construct.

However, the new.targetis much more complicated to backport.
A test like this.constructor === fnmight do the trick in most situation.

If those are acceptable, I'll open a PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants