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

Stop polyfilling Promise (facebook/react-native#29754) #5

Closed
wants to merge 2 commits into from

Conversation

chrisbobbe
Copy link

@chrisbobbe chrisbobbe commented Aug 25, 2020

I've opened facebook#29754 so that this change could eventually land in React Native officially. Note that the facebook/react-native test suite shows failures, there and here (yarn && yarn test), related to Promises. I hope a React Native maintainer will help us figure out what to do about them. Still, it seems to work for our purposes.

This should solve jestjs/jest#10221, letting us use the "modern" implementation of fake timers in Jest.

Since we haven't yet actually used our fork of React Native as our react-native dependency, here are a few notes:

  • To build the Android binaries, I just had to run this Docker command, as instructed in this doc. I suppose we should run this at every commit (can it be automated?):
docker run --rm --name rn-build -v $PWD:/pwd -w /pwd reactnativecommunity/react-native-android /bin/sh -c "./gradlew installArchives"
  • To get them included, I just tweaked the .gitignore to not ignore /android/, as indicated in the instructions (for having the binary live in the repo)

  • No changes to our app project were necessary, beyond pointing the package.json to the appropriate commit on GitHub

  • When identifying a commit in package.json with something other than a commit ID, as in "react-native": "zulip/react-native#0.61.5-zulip" (the "0.61.5-zulip" branch on our fork), we do have to be careful about Yarn caching the code. If the branch changes, it seems that one needs to run yarn cache clean react-native to get the most up-to-date code from the branch upon running yarn.

Following RN's instructions [1].

To build them, we used the recommended Docker command:

```
docker run --rm --name rn-build -v $PWD:/pwd -w /pwd \
    reactnativecommunity/react-native-android /bin/sh -c \
    "./gradlew installArchives"
```

We might have chosen to put the binaries in Maven instead of in the
repository, but that seemed unnecessarily complicated [2].

[1] https://github.com/facebook/react-native/wiki/Building-from-source#publish-your-own-version-of-react-native
[2] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.60react-native.60.20fork/near/984529
It looks like this line was introduced in 3ff3987, in 2015, and it
has remained in a similar form since then. I haven't found any
explanation for it.

At jestjs/jest#10221 [1], a core Jest maintainer says,

"""
As an aside, one should never replace `global.Promise` [...]. E.g.
when using `async-await` you will always get the native `Promise`
regardless of the value of `global.Promise`.
"""

jestjs/jest#10221 is one issue this line has raised, for anyone
using the latest features of Jest to test async code in their React
Native projects.

[1] jestjs/jest#10221 (comment)

Fixes: facebook#29303
@chrisbobbe
Copy link
Author

Closing, as we've found an easier strategy (both mentioned by Greg on a video call, and pointed out in a helpful comment jestjs/jest#10221 (comment)).

@chrisbobbe chrisbobbe closed this Oct 27, 2020
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Oct 27, 2020
This is much easier and less invasive than forking React Native, as
we considered in zulip/react-native#5.

The idea comes from Greg on a video call, with a code example
provided by a @testing-library/react-native contributor [1]. Follow
that example, with tweaks for our project -- using camelCase for the
file names, putting the new files in our `jest` directory, and
adding a TODO mentioning that we'd maybe like to get back to using
jest-expo, which wraps React Native's preset.

[1] jestjs/jest#10221 (comment),
    which links to
    sbalay/without_await@64a76486f.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Jan 11, 2021
This is much easier than forking React Native, as we considered in
zulip/react-native#5.

The idea comes from Greg on a video call, with a code example
provided by a @testing-library/react-native contributor [1]. Follow
that example, with tweaks for our project -- using camelCase for the
file names, putting the new files in our `jest` directory, and
adding a TODO mentioning that we'd maybe like to get back to using
jest-expo, which wraps React Native's preset.

[1] jestjs/jest#10221 (comment),
    which links to
    sbalay/without_await@64a76486f.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Feb 8, 2021
This is much easier than forking React Native, as we considered in
zulip/react-native#5.

The idea comes from Greg on a video call, with a code example
provided by a @testing-library/react-native contributor [1]. Follow
that example, with tweaks for our project -- using camelCase for the
file names, putting the new files in our `jest` directory, and
adding a TODO mentioning that we'd maybe like to get back to using
jest-expo, which wraps React Native's preset.

[1] jestjs/jest#10221 (comment),
    which links to
    sbalay/without_await@64a76486f.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Apr 13, 2021
This is much easier than forking React Native, as we considered in
zulip/react-native#5.

The idea comes from Greg on a video call, with a code example
provided by a @testing-library/react-native contributor [1]. Follow
that example, with tweaks for our project -- using camelCase for the
file names, putting the new files in our `jest` directory, and
adding a TODO mentioning that we'd maybe like to get back to using
jest-expo, which wraps React Native's preset.

[1] jestjs/jest#10221 (comment),
    which links to
    sbalay/without_await@64a76486f.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Apr 13, 2021
This is much easier than forking React Native, as we considered in
zulip/react-native#5.

The idea comes from Greg on a video call, with a code example
provided by a @testing-library/react-native contributor [1]. Follow
that example, with tweaks for our project -- using camelCase for the
file names, putting the new files in our `jest` directory, and
adding a TODO mentioning that we'd maybe like to get back to using
jest-expo, which wraps React Native's preset.

[1] jestjs/jest#10221 (comment),
    which links to
    sbalay/without_await@64a76486f.
@chrisbobbe chrisbobbe deleted the pr-0.61.5-zulip branch April 13, 2021 01:07
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request May 19, 2021
This is much easier than forking React Native, as we considered in
zulip/react-native#5. See
  jestjs/jest#10221 (comment).
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request May 20, 2021
This is much easier than forking React Native, as we considered in
zulip/react-native#5. See
  jestjs/jest#10221 (comment).
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request May 21, 2021
This is much easier than forking React Native, as we considered in
zulip/react-native#5. See
  jestjs/jest#10221 (comment).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant