Skip to content
This repository has been archived by the owner on Apr 13, 2022. It is now read-only.

fix($q): correctly set constructor on prototypes #10697

Closed
wants to merge 3 commits into from

Conversation

bluepnume
Copy link
Contributor

This patch will correctly set Promise.prototype.constructor and Deferred.prototype.constructor.

When prototype is set in the form Foo.prototype = {x: y};, the constructor will not be set correctly (as opposed to in the form Foo.prototype.x = y;.

As such, it is necessary to manually add the constructor to the prototype in these cases.

This patch will correctly set `Promise.prototype.constructor` and `Deferred.prototype.constructor`.

When prototype is set in the form `Foo.prototype = {x: y};`, the constructor will not be set correctly (as opposed to in the form `Foo.prototype.x = y;`.

As such, it is necessary to manually add the constructor to the prototype in these cases.
@googlebot
Copy link

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

@bluepnume
Copy link
Contributor Author

I've signed the cla, please let me know if any other steps need to be completed for this PR to be approved. Thanks!

@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Jan 9, 2015
@lgalfaso
Copy link
Contributor

lgalfaso commented Jan 9, 2015

Why not just

extend(Promise.prototype, {...});

?

@bluepnume
Copy link
Contributor Author

Good suggestion! Done

@lgalfaso
Copy link
Contributor

lgalfaso commented Jan 9, 2015

@bluepnume can you look into the Travis failure, it looks like a valid failure

@bluepnume
Copy link
Contributor Author

I reset the previous commit. It looks like extend is not available at this point in the execution.

@lgalfaso
Copy link
Contributor

lgalfaso commented Jan 9, 2015

@bluepnume look at lib/promises-aplus/promises-aplus-test-adapter.js, some copy & paste should be able to handle the rest

bluepnume added 2 commits January 9, 2015 05:03
Add `setHashKey` and `extend` methods, to allow `$q` to use `extend` to extend its prototype.
Use `extend` on `Promise.prototype` and `Deferred.prototype`, to avoid having to manually set `constructor` on the overwritten prototypes.
@lgalfaso
Copy link
Contributor

Hi,
This needs a little more work before it can land:

  • add some tests
  • rebase on top of master
  • squash everything to a single commit

Thanks!

@lgalfaso lgalfaso added this to the Backlog milestone Jan 20, 2015
@lgalfaso lgalfaso closed this in 3abb3fe Jul 18, 2015
netman92 pushed a commit to netman92/angular.js that referenced this pull request Aug 8, 2015
Use `extend` on `Promise.prototype` and `Deferred.prototype`, to avoid having to
manually set `constructor` on the overwritten prototypes.

Closes angular#10697
ggershoni pushed a commit to ggershoni/angular.js that referenced this pull request Sep 29, 2015
Use `extend` on `Promise.prototype` and `Deferred.prototype`, to avoid having to
manually set `constructor` on the overwritten prototypes.

Closes angular#10697
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants