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

Some application break on specific mobile device or environment. #1098

Closed
wants to merge 1 commit into from
Closed

Conversation

johnny-mh
Copy link

@johnny-mh johnny-mh commented Jun 8, 2018

Pull request number #1041 break app in some mobile browser that Promise.then is not writeable. (We tested on Samsung Galaxy A5) and I agree #1041 code change because it is not writable.

But I think just break some application without any warning message is not right. It's very hard to debugging because error message is undefined is not function and no any useful information like message or stacktrace.

So i add fallback code and display some warning message for that. If you don't want my PR. please suggest other way to notify the user.

Pull request number #1041 break app in some mobile browser that
Promise.then is not writeable. (We tested on Samsung Galaxy A5)
I agree #1041 code change because it is not writable.

But i think just break some application without any warning message
is not right. So i add fallback code and display some wraning
 message for that.
} else {
console.error(`Cannot found Promise#then method. Please add polyfill to using \`Promise\`. If you already did that. Perhaps Promise#then is not writeable. If you want to use it. add following codes to your \`polyfill.ts\` after \`import 'zone.js/dist/zone'\`

if (!(Promise.prototype as any)['__zone_symbol__then']) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

the code should not be added here, because it means the current microTask will not be scheduled. If you think this logic need to be handled, you need to add the logic to patchThen method, but I don't think we need to handle old browser like chrome 34.

Copy link
Author

Choose a reason for hiding this comment

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

this line is not code. just part of template string.

Copy link
Collaborator

@JiaLiPassion JiaLiPassion Jun 8, 2018

Choose a reason for hiding this comment

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

ok. got it, in fact, I forget I already fixed this one in #1079. So please wait for the next release.

Copy link
Author

@johnny-mh johnny-mh Jun 8, 2018

Choose a reason for hiding this comment

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

No #1079 is not initialize Promise.__zone_symbol__then because Promise.then is not patchable OK?

~~The problem is https://github.com/angular/zone.js/pull/1098/files/d4ac887fd112c3fde3a167223c42ecbac76c1f14#diff-dd469785fca8680a5b33b1e81c5cfd91R1276 ~~

nativeMicroTaskQueuePromise is Promise and symbolThen is '__zone_symbol__then. so Invoking Promise. __zone_symbol__then` is always throwing error.

But some case. Promise.then is not patchable but the error message only undefined is not function and no other error message, no error stack trace. in this case It's very hard to find this problem.

It is not problem of only old browser. If some library set Promise.then to not patchable. developer will be face this problem and searching what is the matter of this error message.

Copy link
Author

Choose a reason for hiding this comment

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

@JiaLiPassion Oh #1079 is exactly i want. I will wating next release. 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, sure, thank you for reporting the issue.

@JiaLiPassion
Copy link
Collaborator

JiaLiPassion commented Jun 8, 2018

Duplicate #1079.

@johnny-mh johnny-mh closed this Jun 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants