TypeError: Right-hand side of 'instanceof' is not an object by rifler · Pull Request #38350 · angular/angular

rifler · 2020-08-05T19:43:50Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

Other information

AndrewKushnir · 2020-08-06T01:14:40Z

Hi @rifler, thanks for creating this PR.

It looks like it's missing some critical information to make this PR actionable, specifically:

problem description, which problem this PR resolves
links to open issues (if any)
tests that would verify this change (ideally, the set of tests that would fail without the change)

I will close this PR for now, feel free to reopen or create a new one once you have the necessary information and tests.

Thank you.

rifler · 2020-08-06T08:32:59Z

@AndrewKushnir instanceof is not safe operation, so if you monkeypatch such substantial method as Object.prototype.toString, you have to use try..catch

rifler · 2020-08-06T08:52:53Z

@AndrewKushnir I create an issue - #38361

but cannot reopen pull-request

AndrewKushnir · 2020-08-10T21:44:20Z

@rifler I've re-opened this PR based on #38361 (comment). Thank you.

JiaLiPassion · 2020-08-12T06:47:24Z

packages/zone.js/lib/common/to-string.ts

instead of using try/catch, I think maybe

if (typeof Promise !== 'undefined' && this instanceof Promise) { return PROMISE_OBJECT_TO_STRING; }

will be better, although the code still fails if some app set Promise to some invalid value such as window.Promise = 1, but in that case, the app already fails in a lot of other places.

JiaLiPassion · 2020-08-13T22:48:19Z

@rifler, thanks and year typeof Promise === 'function' is better.
Could you also add a test case to test it?

mhevery · 2020-08-18T18:31:48Z

LGTM after @JiaLiPassion comments are resolved.

rifler · 2020-08-20T19:00:32Z

@JiaLiPassion can you please give me advice how to write unit test for Zone.__load_patch?

Maybe type checking is enough here?

googlebot · 2020-08-21T01:59:47Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

JiaLiPassion · 2020-08-21T03:22:36Z

@rifler , I just added one test case.
And please do the follow cleanups.

git rebase master -i

then squash the current 4 commits to 1 commit.
And then modify the commit message to following the commit guide line here https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
something like

fix(zone.js): zone.js toString patch should check typeof Promise is function

Close #38361

zone.js monkey patch toString, and check the instance is `Promise` or not by using `instanceof Promise`,
sometimes when Promise is not available, the `instanceof` operation fails and throw `TypeError: Right-hand side of 'instanceof' is not an object`, this PR check `typeof Promise` equals to function or not to prevent the error.

…unction Close angular#38361 zone.js monkey patch toString, and check the instance is `Promise` or not by using `instanceof Promise`, sometimes when Promise is not available, the `instanceof` operation fails and throw `TypeError: Right-hand side of 'instanceof' is not an object` this PR check `typeof Promise` equals to function or not to prevent the error.

googlebot · 2020-08-24T17:50:22Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

rifler · 2020-08-24T19:59:09Z

fixed

JiaLiPassion · 2020-08-25T00:38:21Z

LGTM.

AndrewKushnir · 2020-08-25T00:52:53Z

Presubmit

…unction (#38350) Close #38361 zone.js monkey patch toString, and check the instance is `Promise` or not by using `instanceof Promise`, sometimes when Promise is not available, the `instanceof` operation fails and throw `TypeError: Right-hand side of 'instanceof' is not an object` this PR check `typeof Promise` equals to function or not to prevent the error. PR Close #38350

…unction (angular#38350) Close angular#38361 zone.js monkey patch toString, and check the instance is `Promise` or not by using `instanceof Promise`, sometimes when Promise is not available, the `instanceof` operation fails and throw `TypeError: Right-hand side of 'instanceof' is not an object` this PR check `typeof Promise` equals to function or not to prevent the error. PR Close angular#38350

…ise is function (angular#38350)" This reverts commit f4b034d.

…unction (angular#38350) Close angular#38361 zone.js monkey patch toString, and check the instance is `Promise` or not by using `instanceof Promise`, sometimes when Promise is not available, the `instanceof` operation fails and throw `TypeError: Right-hand side of 'instanceof' is not an object` this PR check `typeof Promise` equals to function or not to prevent the error. PR Close angular#38350

angular-automatic-lock-bot · 2020-09-25T16:21:35Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Aug 5, 2020

pullapprove bot requested a review from mhevery August 5, 2020 19:44

AndrewKushnir added the area: zones Issues related to zone.js label Aug 5, 2020

ngbot bot added this to the needsTriage milestone Aug 5, 2020

AndrewKushnir closed this Aug 6, 2020

rifler mentioned this pull request Aug 6, 2020

TypeError: Right-hand side of 'instanceof' is not an object #38361

Closed

AndrewKushnir reopened this Aug 10, 2020

AndrewKushnir requested a review from JiaLiPassion August 10, 2020 21:43

JiaLiPassion suggested changes Aug 12, 2020

View reviewed changes

mhevery approved these changes Aug 18, 2020

View reviewed changes

rifler force-pushed the patch-1 branch from ea4559d to 9ff4779 Compare August 20, 2020 19:00

googlebot added cla: no and removed cla: yes labels Aug 21, 2020

JiaLiPassion added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews target: patch This PR is targeted for the next patch release labels Aug 21, 2020

rifler force-pushed the patch-1 branch from 537419e to 472a716 Compare August 24, 2020 17:50

googlebot added cla: yes and removed cla: no labels Aug 24, 2020

JiaLiPassion self-requested a review August 25, 2020 00:38

JiaLiPassion approved these changes Aug 25, 2020

View reviewed changes

JiaLiPassion removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Aug 25, 2020

AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Aug 25, 2020

AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit labels Aug 25, 2020

mhevery closed this in 18e474f Aug 25, 2020

subratpalhar92 added a commit to SUBRATPALHAR-ALL-JAVASCRIPT/angular that referenced this pull request Aug 31, 2020

Revert "fix(zone.js): zone.js toString patch should check typeof Prom…

59fe5dc

…ise is function (angular#38350)" This reverts commit f4b034d.

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 25, 2020

Conversation

rifler commented Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

AndrewKushnir commented Aug 6, 2020

Uh oh!

rifler commented Aug 6, 2020

Uh oh!

rifler commented Aug 6, 2020

Uh oh!

AndrewKushnir commented Aug 10, 2020

Uh oh!

JiaLiPassion Aug 12, 2020

Choose a reason for hiding this comment

Uh oh!

JiaLiPassion commented Aug 13, 2020

Uh oh!

mhevery commented Aug 18, 2020

Uh oh!

rifler commented Aug 20, 2020

Uh oh!

googlebot commented Aug 21, 2020

Uh oh!

JiaLiPassion commented Aug 21, 2020

Uh oh!

googlebot commented Aug 24, 2020

Uh oh!

rifler commented Aug 24, 2020

Uh oh!

JiaLiPassion commented Aug 25, 2020

Uh oh!

AndrewKushnir commented Aug 25, 2020

Uh oh!

angular-automatic-lock-bot bot commented Sep 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rifler commented Aug 5, 2020 •

edited

Loading