@unimodules/core version constraint seems often incorrect #7728
Labels
Comments
chrisbobbe
pushed a commit
to chrisbobbe/zulip-mobile
that referenced
this issue
Apr 8, 2020
This lets us use individual Expo packages. In particular, we plan to use expo-apple-authentication soon, for in place of some existing dependencies, such as (non-exhaustively) react-native-safe-area and react-native-orientation. So, add it, without yet using it, following instructions at https://github.com/unimodules/react-native-unimodules/blob/30da302a8/README.md. It looks like Unimodules has also gone by the name Universal Modules, but this may be an old name that has been superseded: unimodules/unimodules.org@23c53dd...bdc80d9 Note that we omit the caret in 0.6.0, to disallow silent minor version upgrades. The maintainers are not adhering to semantic versioning, and they expect that you're using the latest version of React Native if you're using the latest version of unimodules, including minor versions; see https://github.com/unimodules/react-native-unimodules/issues/100#issuecomment-566436685. In that same comment, a maintainer says that 0.7.0-rc.4 is the minimum version with AndroidX compatibility. If this is true, then we may have a slight problem, because 0.7.0-rc.4, just like 0.6.0, does not work with RN 0.59.10. That would mean we had to do the AndroidX upgrade (PR zulip#3852) at the same time as the RN v0.60 upgrade (issue zulip#3548). Greg thinks it's likely that the maintainer is mistaken that there's a minimum version of unimodules required to use AndroidX; the AndroidX engineers designed a smooth migration path such that this shouldn't happen (see Greg's comment at zulip#3987 (comment)). A later commit in this series describes another concern with Expo's handling of version constraints, with expo-application (and others), with a reference to expo/expo#7728, which I just filed. A couple of fixes had to be made to get it to work on Android: First, pass an additional argument to a constructor in MainApplication.java, in a fix given by a maintainer at https://github.com/unimodules/react-native-unimodules/issues/128#issuecomment-606027692. Second, specify a new dependency for `unimodules-react-native-adapter` in our own `android/build.gradle`, in a fix that is necessary because we're locked on version 0.6.0 of react-native-unimodules. Filed as https://github.com/unimodules/react-native-unimodules/issues/130. Also (not Android-specific), ran `yarn yarn-deduplicate` to deduplicate `ua-parser-js`, following a prompt from `tools/test deps`. Fixes: zulip#3987
chrisbobbe
pushed a commit
to chrisbobbe/zulip-mobile
that referenced
this issue
Apr 8, 2020
Primarily to prove the correctness of our `react-native-unimodules` installation, install expo-application and use it instead of `react-native-device-info` in those places where it reads the application's version. NOTE: After discussing it at https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/unimodules.20unmet.20peer.20dependency/near/849254, we decided to ignore the following warning: warning " > expo-application@2.1.0" has unmet peer dependency "@unimodules/core@~1.0.0". I filed expo/expo#7728 for it, but basically, the latest version of expo-application (2.1.0) declares that it depends on a version of @unimodules/core (1.0.0) that was already several months out-of-date at the time expo-application was first created, and it seems unlikely that this was well-considered. In our testing so far, everything seems to work correctly, and the vast majority of unimodules don't specify a version constraint at all. But this adds to the need to be wary about Expo's handling of version constraints in general; see the notes on the commit, earlier in this series, that introduces unimodules. To choose between `nativeApplicationVersion` and `nativeBuildVersion`, the deciding factor was which one of these more closely mirrors the behavior of `getVersion` from `react-native-device-info`. (`getVersion` isn't documented well enough to give the answer outright.) `react-native-device-info` is missing documentation on where they get the version number, so we dug into the code and found it: - iOS: `CFBundleShortVersionString` from `info.plist` - Android: `PackageInfo#versionName` (doc at https://developer.android.com/reference/android/content/pm/PackageInfo#versionName) From inspecting `expo-application` code, it turns out that these same exact sources are used for their `nativeApplicationVersion`. The `expo-application` doc for Android actually refers to an `app.json`, which we don't have, and it doesn't mention `versionName`. Maintaining an `app.json` is a normal part of development when you're working entirely in the Expo ecosystem (which we're not), and it's documented (https://docs.expo.io/versions/latest/workflow/configuration/#version) that the "version" key there does indeed correspond to the `versionName` we found by looking in the code. So, use `nativeApplicationVersion`. We don't expect `nativeApplicationVersion` to ever be null, but the type annotation indicates it might be, so handle that case with a "?.?.?" string.
gnprice
pushed a commit
to chrisbobbe/zulip-mobile
that referenced
this issue
Apr 8, 2020
This lets us use individual Expo packages. In particular, we plan to use expo-apple-authentication soon, for in place of some existing dependencies, such as (non-exhaustively) react-native-safe-area and react-native-orientation. So, add it, without yet using it, following instructions at https://github.com/unimodules/react-native-unimodules/blob/30da302a8/README.md. It looks like Unimodules has also gone by the name Universal Modules, but this may be an old name that has been superseded: unimodules/unimodules.org@23c53dd...bdc80d9 Note that we omit the caret in 0.6.0, to disallow silent minor version upgrades. The maintainers are not adhering to semantic versioning, and they expect that you're using the latest version of React Native if you're using the latest version of unimodules, including minor versions; see https://github.com/unimodules/react-native-unimodules/issues/100#issuecomment-566436685. In that same comment, a maintainer says that 0.7.0-rc.4 is the minimum version with AndroidX compatibility. If this is true, then we may have a slight problem, because 0.7.0-rc.4, just like 0.6.0, does not work with RN 0.59.10. That would mean we had to do the AndroidX upgrade (PR zulip#3852) at the same time as the RN v0.60 upgrade (issue zulip#3548). Greg thinks it's likely that the maintainer is mistaken that there's a minimum version of unimodules required to use AndroidX; the AndroidX engineers designed a smooth migration path such that this shouldn't happen (see Greg's comment at zulip#3987 (comment)). A later commit in this series describes another concern with Expo's handling of version constraints, with expo-application (and others), with a reference to expo/expo#7728, which I just filed. A couple of fixes had to be made to get it to work on Android: First, pass an additional argument to a constructor in MainApplication.java, in a fix given by a maintainer at https://github.com/unimodules/react-native-unimodules/issues/128#issuecomment-606027692. Second, specify a new dependency for `unimodules-react-native-adapter` in our own `android/build.gradle`, in a fix that is necessary because we're locked on version 0.6.0 of react-native-unimodules. Filed as https://github.com/unimodules/react-native-unimodules/issues/130. Also (not Android-specific), ran `yarn yarn-deduplicate` to deduplicate `ua-parser-js`, following a prompt from `tools/test deps`. Fixes: zulip#3987
gnprice
pushed a commit
to chrisbobbe/zulip-mobile
that referenced
this issue
Apr 8, 2020
Primarily to prove the correctness of our `react-native-unimodules` installation, install expo-application and use it instead of `react-native-device-info` in those places where it reads the application's version. NOTE: After discussing it at https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/unimodules.20unmet.20peer.20dependency/near/849254, we decided to ignore the following warning: warning " > expo-application@2.1.0" has unmet peer dependency "@unimodules/core@~1.0.0". I filed expo/expo#7728 for it, but basically, the latest version of expo-application (2.1.0) declares that it depends on a version of @unimodules/core (1.0.0) that was already several months out-of-date at the time expo-application was first created, and it seems unlikely that this was well-considered. In our testing so far, everything seems to work correctly, and the vast majority of unimodules don't specify a version constraint at all. But this adds to the need to be wary about Expo's handling of version constraints in general; see the notes on the commit, earlier in this series, that introduces unimodules. To choose between `nativeApplicationVersion` and `nativeBuildVersion`, the deciding factor was which one of these more closely mirrors the behavior of `getVersion` from `react-native-device-info`. (`getVersion` isn't documented well enough to give the answer outright.) `react-native-device-info` is missing documentation on where they get the version number, so we dug into the code and found it: - iOS: `CFBundleShortVersionString` from `info.plist` - Android: `PackageInfo#versionName` (doc at https://developer.android.com/reference/android/content/pm/PackageInfo#versionName) From inspecting `expo-application` code, it turns out that these same exact sources are used for their `nativeApplicationVersion`. The `expo-application` doc for Android actually refers to an `app.json`, which we don't have, and it doesn't mention `versionName`. Maintaining an `app.json` is a normal part of development when you're working entirely in the Expo ecosystem (which we're not), and it's documented (https://docs.expo.io/versions/latest/workflow/configuration/#version) that the "version" key there does indeed correspond to the `versionName` we found by looking in the code. So, use `nativeApplicationVersion`. We don't expect `nativeApplicationVersion` to ever be null, but the type annotation indicates it might be, so handle that case with a "?.?.?" string.
|
hey @chrisbobbe! I have changed the peer dependencies for all of the expo packages to use a lenient |
|
Yay, thanks, @brentvatne! |
chrisbobbe
pushed a commit
to chrisbobbe/zulip-mobile
that referenced
this issue
Apr 9, 2020
On expo-application v2.1.0 (see 18c37ce), we were seeing this warning on initial runs of `yarn`: warning " > expo-application@2.1.0" has unmet peer dependency "@unimodules/core@~1.0.0". (Same as in 5b6014e: "No such warning if `yarn` stops after step 'Resolving packages' and prints 'Already up-to-date'; but if it has any actual work to do, it always shows this warning in step 'Linking dependencies'.) Version 2.1.0 of expo-application declares that it depends on a version of @unimodules/core (1.0.0) that was already several months out-of-date at the time expo-application was first created, and it seems unlikely that this was well-considered. I filed expo/expo#7728 for this, and it was promptly addressed by releasing version 2.1.1 of expo-application that had "*" for its version constraint on the @unimodules/core peer dependency. So, switch to that new version. Even when we do that, though, we get the following: warning " > expo-application@2.1.1" has unmet peer dependency "@unimodules/core@*". This goes away when we declare a version of @unimodules/core in our own dependencies, though. Greg did some digging and concluded that this is likely what we're supposed to do, actually, because it's declared as a "peer dependency": """ Here's the reference doc for it: https://docs.npmjs.com/files/package.json#peerdependencies but it's not very clear about what it actually *means* -- mostly it just describes a use-case. The closest I've found to clear docs on it may be this Yarn blog post: https://classic.yarnpkg.com/blog/2018/04/18/dependencies-done-right/ > The `peerDependencies` object guarantees that, for each entry, any > package that requires you will be requested to provide a copy of > the dependency listed here, ideally matching the version you > requested. It also guarantees that you’ll be able to access this > dependency through `require()`, and finally **it also guarantees > that the return of `require()` will be exactly the same version & > instance than the one your parent would obtain if they were to > `require()` it themselves**. [emphasis in original] And if you work through the consequences of that... it means in particular that the "parent" has to have that dependency and be able to `require()` it themselves. Which, if you're declaring your dependencies cleanly, means the parent should have it as a dependency in their own `package.json`. """ So, declare @unimodules/core in our own "dependencies", with the version specified by react-native-unimodules in its active version. This matching should be done each time we change versions of react-native-unimodules.
chrisbobbe
pushed a commit
to chrisbobbe/zulip-mobile
that referenced
this issue
Apr 10, 2020
On expo-application v2.1.0 (see 18c37ce), we were seeing this warning on initial runs of `yarn`: warning " > expo-application@2.1.0" has unmet peer dependency "@unimodules/core@~1.0.0". (Same as in 5b6014e: "No such warning if `yarn` stops after step 'Resolving packages' and prints 'Already up-to-date'; but if it has any actual work to do, it always shows this warning in step 'Linking dependencies'.) Version 2.1.0 of expo-application declares that it depends on a version of @unimodules/core (1.0.0) that was already several months out-of-date at the time expo-application was first created, and it seems unlikely that this was well-considered. I filed expo/expo#7728 for this, and it was promptly addressed by releasing version 2.1.1 of expo-application that had "*" for its version constraint on the @unimodules/core peer dependency. So, switch to that new version. Even when we do that, though, we get the following: warning " > expo-application@2.1.1" has unmet peer dependency "@unimodules/core@*". This goes away when we declare a version of @unimodules/core in our own dependencies, though. Greg did some digging and concluded that this is likely what we're supposed to do, actually, because it's declared as a "peer dependency": """ Here's the reference doc for it: https://docs.npmjs.com/files/package.json#peerdependencies but it's not very clear about what it actually *means* -- mostly it just describes a use-case. The closest I've found to clear docs on it may be this Yarn blog post: https://classic.yarnpkg.com/blog/2018/04/18/dependencies-done-right/ > The `peerDependencies` object guarantees that, for each entry, any > package that requires you will be requested to provide a copy of > the dependency listed here, ideally matching the version you > requested. It also guarantees that you’ll be able to access this > dependency through `require()`, and finally **it also guarantees > that the return of `require()` will be exactly the same version & > instance than the one your parent would obtain if they were to > `require()` it themselves**. [emphasis in original] And if you work through the consequences of that... it means in particular that the "parent" has to have that dependency and be able to `require()` it themselves. Which, if you're declaring your dependencies cleanly, means the parent should have it as a dependency in their own `package.json`. """ So, declare @unimodules/core in our own "dependencies", with the version specified by react-native-unimodules in its active version. This matching should be done each time we change versions of react-native-unimodules.
chrisbobbe
pushed a commit
to chrisbobbe/zulip-mobile
that referenced
this issue
Apr 10, 2020
On expo-application v2.1.0 (see 18c37ce), we were seeing this warning on initial runs of `yarn`: warning " > expo-application@2.1.0" has unmet peer dependency "@unimodules/core@~1.0.0". (Same as in 5b6014e: "No such warning if `yarn` stops after step 'Resolving packages' and prints 'Already up-to-date'; but if it has any actual work to do, it always shows this warning in step 'Linking dependencies'.) Version 2.1.0 of expo-application declares that it depends on a version of @unimodules/core (1.0.0) that was already several months out-of-date at the time expo-application was first created, and it seems unlikely that this was well-considered. I filed expo/expo#7728 for this, and it was promptly addressed by releasing version 2.1.1 of expo-application that had "*" for its version constraint on the @unimodules/core peer dependency. So, switch to that new version. Even when we do that, though, we get the following: warning " > expo-application@2.1.1" has unmet peer dependency "@unimodules/core@*". This goes away when we declare a version of @unimodules/core in our own dependencies, though. Greg did some digging and concluded that this is likely what we're supposed to do, actually, because it's declared as a "peer dependency": """ Here's the reference doc for it: https://docs.npmjs.com/files/package.json#peerdependencies but it's not very clear about what it actually *means* -- mostly it just describes a use-case. The closest I've found to clear docs on it may be this Yarn blog post: https://classic.yarnpkg.com/blog/2018/04/18/dependencies-done-right/ > The `peerDependencies` object guarantees that, for each entry, any > package that requires you will be requested to provide a copy of > the dependency listed here, ideally matching the version you > requested. It also guarantees that you’ll be able to access this > dependency through `require()`, and finally **it also guarantees > that the return of `require()` will be exactly the same version & > instance than the one your parent would obtain if they were to > `require()` it themselves**. [emphasis in original] And if you work through the consequences of that... it means in particular that the "parent" has to have that dependency and be able to `require()` it themselves. Which, if you're declaring your dependencies cleanly, means the parent should have it as a dependency in their own `package.json`. """ So, declare @unimodules/core in our own "dependencies", with the version specified by react-native-unimodules in its active version. This matching should be done each time we change versions of react-native-unimodules.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
If this is already being tracked, please close this and consider folding some of these findings in as a bullet point.
In a bare project, if I have
@unimodules/coreat version 4.0.0, and Iyarn add expo-application, I geton this and subsequent runs of
yarn. I have@unimodules/coreat version 4.0.0 (I believe this isn't the latest, but we're stuck onreact-native-unimodulesat 0.6.0, no caret, because we're still on React Native v0.59.10. The need for 0.6.0 is explained here).In any case, it looks like version 2.1.0 of
expo-application— which is its latest version! — declares that it won't work unless you're on version ~1.0.0 of@unimodules/core, which was superseded in April 2019 (shown withgit log --stat -p -S 2.0.0 packages/@unimodules/core/package.json).In fact, at the time
expo-applicationwas first created, in August 2019,@unimodules/corewas already at version 3.0.2!From experimentation, the 1.0.0 version constraint doesn't seem to be meaningful; we haven't, yet, run into problems with
expo-application. But we've so far maintained that the output ofyarnis warning-free, to make sure there's a clear signal there, for our own contributors, so it would be a great improvement if this noise (if it is noise) were removed.Here's some data on the unimodules' version constraints for
@unimodules/core.So, it looks like most unimodules are lenient, and don't have any meaningful version constraint. Can this be applied across the board? Or, at least, could all new releases of a package be made and declared compatible with current, commonly used versions of
@unimodules/core?The text was updated successfully, but these errors were encountered: