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

deps: Upgrade jest-expo to 42.1.0, the latest. #4928

Merged
merged 1 commit into from
Sep 1, 2021

Conversation

chrisbobbe
Copy link
Contributor

@chrisbobbe chrisbobbe commented Jul 29, 2021

This is a bump of two major versions. Nothing seems to go wrong (and if it did, I'd expect it to only affect Jest tests, and those still all pass). But in addition to removing a hack described in the commit message, this will let us iron out any wrinkles from those major version bumps soon, so we don't have to deal with them at the time of upgrading to a potential newer version that has explicit RN v0.64 support, with something like expo/expo#13549, for #4426.

@chrisbobbe chrisbobbe added the dependencies Pull requests that update a dependency file label Jul 29, 2021
@chrisbobbe
Copy link
Contributor Author

chrisbobbe commented Jul 29, 2021

Also, this bit of output from git log --stat is kind of nice:

 yarn.lock    | Bin 549096 -> 512179 bytes

@gnprice
Copy link
Member

gnprice commented Aug 11, 2021

LGTM, thanks!

There's a rebase conflict in yarn.lock, and I'm getting puzzling results when I try to resolve it.

  • First, I tried using yarn's feature for this: on hitting the conflict, ran yarn, then continued the rebase.
  • Then, I tried the direct approach: take just the package.json change atop current master, and run yarn from there.

With either of those, I found that yarn.lock ended up with two versions of @expo/config-plugins: a 2.0.4, and a 3.0.7. Your PR branch has only a 3.0.7. (Before the branch, it's two versions 1.0.33 and 3.0.6.)

Having just one config sounds like a good thing. Here's from yarn why @expo/config-plugins:

=> Found "@expo/config-plugins@3.0.6"
info Has been hoisted to "@expo/config-plugins"
info Reasons this module exists
   - Hoisted from "expo-apple-authentication#@expo#config-plugins"
   - Hoisted from "expo-screen-orientation#@expo#config-plugins"
   - Hoisted from "react-native-unimodules#expo-file-system#@expo#config-plugins"

=> Found "@expo/config#@expo/config-plugins@3.0.7"
info This module exists because "jest-expo#@expo#config" depends on it.

=> Found "expo-constants#@expo/config-plugins@2.0.4"
info Reasons this module exists
   - "react-native-unimodules#expo-constants#@expo#config" depends on it
   - Hoisted from "react-native-unimodules#expo-constants#@expo#config#@expo#config-plugins"

(Hmm, so I guess it's actually three versions, though 3.0.6 vs 3.0.7 is probably fine.)

Would you try to reproduce whatever you did to get the yarn.lock changes you got here the first time? 🙂

@chrisbobbe
Copy link
Contributor Author

I'm away from my computer right now, but from that output it looks like our recent upgrade of react-native-unimodules might be a factor.

@chrisbobbe
Copy link
Contributor Author

chrisbobbe commented Aug 13, 2021

I'm away from my computer right now, but from that output it looks like our recent upgrade of react-native-unimodules might be a factor.

Hmm OK and react-native-unimodules looks like it was deprecated 2 hours ago; they're saying we should use expo-modules-core instead: https://github.com/expo/expo/tree/master/packages/react-native-unimodules.

Hoping to get more clarification here: expo/expo#14048 (comment) 🙂

@chrisbobbe
Copy link
Contributor Author

chrisbobbe commented Aug 31, 2021

Hoping to get more clarification here: expo/expo#14048 (comment) 🙂

Hmm, and it looks like that migration is still in progress on Expo's end.

@gnprice, in your comment here—

Having just one config sounds like a good thing.

—was there something specific you were concerned about (like an error) or do you think it might be OK to go ahead with this PR without doing anything special with react-native-unimodules or expo-modules-core?

@chrisbobbe
Copy link
Contributor Author

chrisbobbe commented Sep 1, 2021

Just rebased. Now there's just two (after running yarn yarn-deduplicate && yarn):

$ yarn why @expo/config-plugins
yarn why v1.22.10
[1/4] 🤔  Why do we have the module "@expo/config-plugins"...?
[2/4] 🚚  Initialising dependency graph...
[3/4] 🔍  Finding dependency...
[4/4] 🚡  Calculating file sizes...
=> Found "@expo/config-plugins@3.1.0"
info Has been hoisted to "@expo/config-plugins"
info Reasons this module exists
   - Hoisted from "expo-apple-authentication#@expo#config-plugins"
   - Hoisted from "expo-screen-orientation#@expo#config-plugins"
   - Hoisted from "jest-expo#@expo#config#@expo#config-plugins"
   - Hoisted from "react-native-unimodules#expo-file-system#@expo#config-plugins"
info Disk size without dependencies: "2.45MB"
info Disk size with unique dependencies: "4.49MB"
info Disk size with transitive dependencies: "7.98MB"
info Number of shared dependencies: 52
=> Found "expo-constants#@expo/config-plugins@2.0.4"
info Reasons this module exists
   - "react-native-unimodules#expo-constants#@expo#config" depends on it
   - Hoisted from "react-native-unimodules#expo-constants#@expo#config#@expo#config-plugins"
info Disk size without dependencies: "1.38MB"
info Disk size with unique dependencies: "3.27MB"
info Disk size with transitive dependencies: "6.81MB"
info Number of shared dependencies: 51
✨  Done in 0.77s.

Still, you may still be concerned about that difference in major versions.

@gnprice
Copy link
Member

gnprice commented Sep 1, 2021

Well, I'm not entirely sure what @expo/config-plugins does. It sounds like it's providing configuration of some kind -- and if different parts of our dependency tree are working from configurations that don't agree, that seems like a recipe for bugs.

Reading that package's docs -- https://docs.expo.dev/guides/config-plugins/ , following the link at https://www.npmjs.com/package/@expo/config-plugins -- I think it's OK, though. This isn't so much providing configuration of Expo (or of its "plugins"?); rather it's providing some generic building blocks for writing bits of code ("config plugins") that in turn do things to the Expo config. IOW this package is just providing some code to do a job, and the "config" in its name is just because that job happens to relate to manipulating the Expo config.

It'd still be nice for all the different parts of our dependency tree to be using an up-to-date version, but no more so than for any other dependency.

I'll rebase and merge.

This gets us expo/expo#12735, which lets us use Jest 26, instead of
25, without the `resolutions` hack we added in f1b59fe. So, remove
that hack.
@gnprice gnprice merged commit 3f1fcab into zulip:master Sep 1, 2021
@chrisbobbe
Copy link
Contributor Author

Thanks for the review!

@chrisbobbe chrisbobbe mentioned this pull request Sep 8, 2021
@chrisbobbe chrisbobbe deleted the pr-jest-expo-42-1-0 branch November 4, 2021 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants