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

replace instanceof Promise with check of then function on the object #4122

Merged
merged 2 commits into from
Oct 23, 2020

Conversation

pyu10055
Copy link
Collaborator

@pyu10055 pyu10055 commented Oct 23, 2020

In Angular Promise objects are monkey patched for zone.js
The type check object instanceof Promise will return false.
This PR created a util method that check the type of then field of the object instead.

fix #3494

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

Copy link
Collaborator

@lina128 lina128 left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @lina128 and @pyu10055)


tfjs-core/src/util.ts, line 800 at r1 (raw file):

/**
 * This method to check of the object can be interpreted as `Promise` instance.

This method asserts whether an object is a Promise instance.


tfjs-core/src/util.ts, line 802 at r1 (raw file):

 * Since `instanceof Promise` only works for es6 `Promise`, some framework like
 * Angular has custom promise where `instanceof Promise` return false.

Should we move this part inside the function? Because it is implementation detail.


tfjs-core/src/util.ts, line 806 at r1 (raw file):

 */
export function isPromise(object: any) {
  return object && object.then && typeof object.then === 'function';

Add the annotation here, something like:
// We chose to not use 'obj instanceOf Promise' for two reasons: 1. It only reliably works for es6 Promise, not other Promise implementations. 2. It doesn't work with framework that uses zone.js. zone.js monkey patch the async calls, so it is possible the obj (patched) is comparing to a pre-patched Promise.

Copy link
Collaborator Author

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @lina128)


tfjs-core/src/util.ts, line 800 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

This method asserts whether an object is a Promise instance.

Done.


tfjs-core/src/util.ts, line 802 at r1 (raw file):

Previously, lina128 (Na Li) wrote…
 * Since `instanceof Promise` only works for es6 `Promise`, some framework like
 * Angular has custom promise where `instanceof Promise` return false.

Should we move this part inside the function? Because it is implementation detail.

Done.


tfjs-core/src/util.ts, line 806 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Add the annotation here, something like:
// We chose to not use 'obj instanceOf Promise' for two reasons: 1. It only reliably works for es6 Promise, not other Promise implementations. 2. It doesn't work with framework that uses zone.js. zone.js monkey patch the async calls, so it is possible the obj (patched) is comparing to a pre-patched Promise.

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem with Inferencing with Custom Object Detection Model
2 participants