Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix($q): allow third-party promises #16471

Closed
wants to merge 1 commit into from

Conversation

vbraun
Copy link
Contributor

@vbraun vbraun commented Feb 26, 2018

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
For testing, it can be useful to run with other promise
implementations like Bluebird.js + angular-bluebird-promises. This
broke in angularjs 1.6 with a "TypeError: Cannot set property 'pur' of
undefined".

Closes #16164

What is the current behavior? (You can also link to an open issue here)
Raises TypeError: Cannot set property 'pur' of undefined

What is the new behavior (if this is a feature change)?
N/A

Does this PR introduce a breaking change?
No

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Fix/Feature: Docs have been added/updated
  • Fix/Feature: Tests have been added; existing tests pass

Other information:

For testing, it can be useful to run with other promise
implementations like Bluebird.js + angular-bluebird-promises. This
broke in angularjs 1.6 with a "TypeError: Cannot set property 'pur' of
undefined".

Closes angular#16164
@petebacondarwin
Copy link
Contributor

petebacondarwin commented Feb 26, 2018

@vbraun - thanks for sending this in.
Have you tested that this then works with Bluebird? I wonder if we could have some kind of tests for this?

@vbraun
Copy link
Contributor Author

vbraun commented Feb 26, 2018

I'm using kriskowal's Q with jsdom for some tests, and I can confirm that it works with that combination. On #16164 the reporter indicated that it works with Bluebird but I haven't confirmed that personally.

Not sure about unit tests, of course one could try various third-party libraries but would be quite some effort... Or just manually delete the q.$$state?

@gkalpak
Copy link
Member

gkalpak commented Mar 5, 2018

We decided it is not worth adding tests for the specific issue that this PR solves, since substituting $q with another promise library is neither thoroughly tested, nor officially supported (although it seems to be possible). So, even if we tested that nothing breaks due to missing $$state, there is still no guarrantee that something else doesn't break.

We can land this as is (with a comment explaining what these lines guard against).

@@ -677,10 +677,11 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) {
}

function isStateExceptionHandled(state) {
if (!state) return true;
Copy link
Member

@gkalpak gkalpak Mar 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm...I was going through the code looking for other places that might need fixing and I realized I am not sure why this should be necessary (i.e. where this function would be called from when using a 3rd-party promise library) 😕

Could you share a minimal demo (e.g. plnkr, repo, or live app (with non-minified sources 😁)) that showcases the current failures?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this change is indeed not necessary (relevant discussion).

Copy link

@davidwilder davidwilder Oct 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you share a minimal demo (e.g. plnkr, repo, or live app (with non-minified sources 😁)) that showcaes the current failures?

We are also using using Bluebird.js + angular-bluebird-promises. Setting ngModelOptions raises the TypeError: Cannot set property 'pur' of undefined. Please find a demo here http://plnkr.co/edit/9oDzLt?p=preview

Copy link
Member

@gkalpak gkalpak Nov 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx for the reproduction, @davidwilder. Based on the stack trace, the error happens in the markQStateExceptionHandled() method and not isStateExceptionHandled(). As mentioned in #16164 (comment), I don't think isStateExceptionHandled() needs to be changed (but it won't hurt either).

@gkalpak
Copy link
Member

gkalpak commented Nov 15, 2018

Tweaked a bit and merged.

BTW, if anyone can't upgrade to the latest AngularJS version, it is possible to work around this issue by adding a $$state: {} property on the promise prototype. For example, in the case of Bluebird + angular-bluebird-promises (demo):

.config(Bluebird => Bluebird.prototype.$$state = {})

gkalpak pushed a commit that referenced this pull request Nov 15, 2018
For testing, it can be useful to overwrite `$q` with other promise
implementions, such as Bluebird + angular-bluebird-promises. This broke
in v1.6.x with "TypeError: Cannot set property 'pur' of undefined".

Closes #16164

Closes #16471
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.

Cannot set property 'pur' of undefined
5 participants