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

Ensure that the return value of apiFetch is always a valid Promise object in Firefox #10214

Merged
merged 2 commits into from Sep 30, 2018

Conversation

Projects
None yet
5 participants
@youknowriad
Contributor

youknowriad commented Sep 27, 2018

it looks like window.fetch in firefox don't return an object that is instance of Promise causing some weird issues down the road.

This PR ensures we always return a promise object.

closes #10208
fixes #10265

Testing instructions

  • Try embedding a tweet in Firefox, you should see the preview.

@youknowriad youknowriad self-assigned this Sep 27, 2018

@youknowriad youknowriad requested review from aduth, notnownikki and WordPress/gutenberg-core Sep 27, 2018

@notnownikki

Embeds work! Fetching status works! Thank you <3

I'll leave it up to @aduth for the approval though, I'm not 100% sure of the repercussions of this.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 27, 2018

Member

it looks like window.fetch in firefox don't return an object that is instance of Promise

I'd like to know more.

Member

aduth commented Sep 27, 2018

it looks like window.fetch in firefox don't return an object that is instance of Promise

I'd like to know more.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 27, 2018

Contributor

@aduth I really don't know more about it. maybe some polyfill related thing. Not sure what can I do. All I know is window.fetch('something') instanceof Promise === false

Contributor

youknowriad commented Sep 27, 2018

@aduth I really don't know more about it. maybe some polyfill related thing. Not sure what can I do. All I know is window.fetch('something') instanceof Promise === false

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 27, 2018

Member

If I'm known for one thing, it's for my nagging relentlessness in not allowing "I don't know" to pass 😄

It is an issue related to polyfills. Specifically, the native promise is overridden in Firefox due to lack of support of a PromiseRejectionEvent global. Fetch is not polyfilled, however, so there is a mismatch between the promise as returned by fetch and the promise global. You can see this mentioned in comments of this StackOverflow answer, which also explains why the standard redux middleware for promises uses a library which to detect promises by shape, not reference.

So, I think this is more an issue of how we detect things as promises (in our promise middleware) rather than the return value of fetch, which is in-fact a promise.

Member

aduth commented Sep 27, 2018

If I'm known for one thing, it's for my nagging relentlessness in not allowing "I don't know" to pass 😄

It is an issue related to polyfills. Specifically, the native promise is overridden in Firefox due to lack of support of a PromiseRejectionEvent global. Fetch is not polyfilled, however, so there is a mismatch between the promise as returned by fetch and the promise global. You can see this mentioned in comments of this StackOverflow answer, which also explains why the standard redux middleware for promises uses a library which to detect promises by shape, not reference.

So, I think this is more an issue of how we detect things as promises (in our promise middleware) rather than the return value of fetch, which is in-fact a promise.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 27, 2018

Contributor

So, I think this is more an issue of how we detect things as promises (in our promise middleware) rather than the return value of fetch, which is in-fact a promise.

That was my original way of fixing the issue, but we do test promises in several places. Should we update them all?

What should we consider a promise: an object with a .then function?

I did try to search for the issue elsewhere but failed. You're more skilled then me :)

Contributor

youknowriad commented Sep 27, 2018

So, I think this is more an issue of how we detect things as promises (in our promise middleware) rather than the return value of fetch, which is in-fact a promise.

That was my original way of fixing the issue, but we do test promises in several places. Should we update them all?

What should we consider a promise: an object with a .then function?

I did try to search for the issue elsewhere but failed. You're more skilled then me :)

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 27, 2018

Member

What should we consider a promise: an object with a .then function?

I think this is safest in reality where promises are often polyfilled, even if it is not quite as semantic as testing it against the Promise global. It might be a touch better for us than in delegating this to the things which produce promises to normalize (i.e. apiFetch).

Ultimately, It's a small difference that I don't care too much one way or the other. My bigger concern was having the understanding of what was causing the issue, so we could at least be well-informed to be able to evaluate the best solution.

Member

aduth commented Sep 27, 2018

What should we consider a promise: an object with a .then function?

I think this is safest in reality where promises are often polyfilled, even if it is not quite as semantic as testing it against the Promise global. It might be a touch better for us than in delegating this to the things which produce promises to normalize (i.e. apiFetch).

Ultimately, It's a small difference that I don't care too much one way or the other. My bigger concern was having the understanding of what was causing the issue, so we could at least be well-informed to be able to evaluate the best solution.

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Sep 27, 2018

Member

This is affecting more then I thought in firefox. For example, categories and tags don't appear in the document settings without this patch.

Member

notnownikki commented Sep 27, 2018

This is affecting more then I thought in firefox. For example, categories and tags don't appear in the document settings without this patch.

@tofumatt

Thanks for digging into this @aduth. If the "intuitive" way to check for Promises is problematic, seems like we should be relying on a library (like other projects do) to detect Promises. @aduth mentioned: https://www.npmjs.com/package/is-promise... I think it makes more sense to update our checks to use something like that is-promise library. I worry otherwise we'll just run into edge cases like this again.

It might be nice to enforce a rule about it if possible, maybe alert if you are checking for instanceof Promise or whatever, but that can happen later; I'm not sure how we test across the repo.

@tofumatt tofumatt added this to the 4.0 milestone Sep 29, 2018

@tofumatt

This is great and also fixes #10265. 🚢

@youknowriad youknowriad merged commit 7509ace into master Sep 30, 2018

2 checks passed

codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +51.42% compared to 0fe7d08
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the fix/fetch-promise-firefox branch Sep 30, 2018

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Oct 1, 2018

Member

This is affecting more then I thought in firefox. For example, categories and tags don't appear in the document settings without this patch.

I think it also impacted #9982

Member

danielbachhuber commented Oct 1, 2018

This is affecting more then I thought in firefox. For example, categories and tags don't appear in the document settings without this patch.

I think it also impacted #9982

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment