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.

Sign up for GitHub

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

Jump to bottom

Set Concast["@@species"] = Observable as well as Symbol.species. #7403

Merged
merged 1 commit into from
Dec 3, 2020
Merged

Set Concast["@@species"] = Observable as well as Symbol.species. #7403

merged 1 commit into from
Dec 3, 2020

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Dec 2, 2020 •
edited
Loading

This is an initial attempt to address the problems I speculated about in #6520 (comment).

Theory: in the Hermes JS engine (used by React Native on Android), Symbol.species is not defined, so Object.defineProperty was not called by this code, but perhaps Concast["@@species"] was getting set somewhere else by a polyfill library, causing the zen-observable library to fall back to "@@species" in getSpecies instead of safely ignoring the nonexistent Concast[Symbol.species] property.

If that theory is correct, subscriptions will become usable with Hermes on React Native again, fixing #6520. 🤞

@benjamn
Set Concast["@@species"] = Observable as well as Symbol.species.
c8cdc6e 
An attempt to address the problems I speculated about in
#6520 (comment)

Theory: in the Hermes JS engine, Symbol.species is not defined, so
Object.defineProperty was not called, but perhaps "@@species" was getting
set somewhere else by a polyfill library, causing the zen-observable
library to fall back to "@@species" instead of using/ignoring the
nonexistent Symbol.species symbol.
@benjamn benjamn added this to the Release 3.4 milestone Dec 2, 2020
@benjamn benjamn self-assigned this Dec 2, 2020
@marcel-happyfloat
Copy link

Thank you very much for your effort! I try to test this patch in the near future.

@hannojg
Copy link

hannojg commented Dec 2, 2020

Using Hermes on android this hasn't fixed the issue for me, unfortunately.

@benjamn
Copy link
Member Author

benjamn commented Dec 2, 2020

@hannojg Did you build your own @apollo/client with this PR applied?

In any case, this change should be in the next v3.4 beta release, so it's easier to install locally.

I think there might be more than one problem under discussion in #6520, by the way. If this PR fixes anyone's problems, then we can focus on the other problems after that, without the Symbol.species distraction.

@watadarkstar
Copy link

Thank you @benjamn for fixing this. We will help confirm if this fixes our subscriptions on Android with Hermes.

@vuongngo
Copy link

vuongngo commented Dec 3, 2020

I've tested this change on our app. Works well when hermes is enabled. Thanks @benjamn 👏 , this is a great fix.

hwillson
hwillson approved these changes Dec 3, 2020
View reviewed changes
Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Wow, amazing catch and fix @benjamn. Thanks!

@hannojg
Copy link

hannojg commented Dec 3, 2020

@benjamn yes I did, happy to test this again when there is a public release (building the Apollo client manually and using this caused other warning which the console throw at me, probably messed something up)

@benjamn benjamn merged commit c8cdc6e into release-3.4 Dec 3, 2020
benjamn added a commit that referenced this pull request Dec 3, 2020
@benjamn
Set both Symbol.species and "@@species" on Concast constructor (#7403)
2a2843c 
An attempt to address the problems I speculated about in
#6520 (comment)

Theory: in the Hermes JS engine, Symbol.species is not defined, so
Object.defineProperty was not called, but perhaps "@@species" was getting
set somewhere else by a polyfill library, causing the zen-observable
library to fall back to "@@species" instead of using/ignoring the
nonexistent Symbol.species symbol.
@benjamn benjamn mentioned this pull request Dec 3, 2020
Closed
benjamn added a commit that referenced this pull request Feb 5, 2021
@benjamn
Set both Symbol.species and "@@species" on Concast constructor (#7403)
ab6b60a 
An attempt to address the problems I speculated about in
#6520 (comment)

Theory: in the Hermes JS engine, Symbol.species is not defined, so
Object.defineProperty was not called, but perhaps "@@species" was getting
set somewhere else by a polyfill library, causing the zen-observable
library to fall back to "@@species" instead of using/ignoring the
nonexistent Symbol.species symbol.
@benjamn benjamn mentioned this pull request Feb 5, 2021
Merged
benjamn added a commit that referenced this pull request Feb 5, 2021
@benjamn
Mention PRs #7403 and #7660 in CHANGELOG.md.
fc57af8
@benjamn
Copy link
Member Author

benjamn commented Feb 5, 2021

This change has been backported to @apollo/[email protected], in case anyone was waiting for it (and unwilling/unable to use the v3.4 betas).

This was referenced Mar 11, 2021
Merged
Closed
@quanndh
Copy link

quanndh commented Mar 18, 2021 •
edited
Loading

For some reason, subcription not working for me =( loading state true, data and error are undefined. Im using apollo client 3.3.8. Im got data on playground however

@hwillson hwillson removed this from the MM-2021-06 milestone Jul 29, 2021
@agatharejina
Copy link

I've tested this change on our app. Works well when hermes is enabled. Thanks @benjamn 👏 , this is a great fix.

👋! can you show me the npm packages regarding the apollo/client and react native package?

I have already tried installing @apollo/[email protected] and @apollo/[email protected]
but, it still won't work everytime I run assembleRelease with hermes enabled

there are my current packages

"@apollo/client": "^3.5.5",
 "react": "16.13.1",
"react-native": "0.63.4",

Are there any solutions to this? thanks before.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hwillson hwillson hwillson approved these changes

@jcreighton jcreighton Awaiting requested review from jcreighton

Assignees

@benjamn benjamn

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@benjamn @marcel-happyfloat @hannojg @watadarkstar @vuongngo @quanndh @agatharejina @hwillson