-
Notifications
You must be signed in to change notification settings - Fork 6.2k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[client] Update NewManifest field paths for new extra field format #13398
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The review previously left here is no longer valid, jump to the latest one 👉 #13398 (review)
a146a41
to
4a4e034
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The review previously left here is no longer valid, jump to the latest one 👉 #13398 (review)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great, although left a question about expoClientConfig
in the other PR.
packages/expo-updates/android/src/main/java/expo/modules/updates/manifest/raw/RawManifest.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me, pending the discussion about the names of these fields in expo/expo-cli#3606.
// use commitTime instead of publishedTime as it is more accurate; | ||
// however, fall back to publishedTime in case older cached manifests do not contain | ||
// the commitTime key (we have not always served it) | ||
return getCommitTime() ?: getPublishedTime() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should be able to safely remove this fallback, as we've been serving both fields for the entirety of the lifetimes of all SDK versions we currently support in Expo Go. I don't think the expo-updates runtime even looks at publishedTime
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4a4e034
to
c1b33f4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The review previously left here is no longer valid, jump to the latest one 👉 #13398 (review)
Failing test run tests are broken in master, landing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines.
Looks like I have nothing to complain about 👏 Keep up the good work! 💪
Generated by ExpoBot 🤖 against 9d6afdd
Why
expo/expo-cli#3606 defines the new format for the
extra
field.expoClientConfig
contains the fields necessary for the client to function (with the exception of scopeKey).expoGoConfig
contains the fields necessary for development using Expo Go.This PR updates the RawManifest and NewManifest to look in the new location for the fields it needs. Previously none of these fields were defined on the new manifest format.
How
Point field getters to new location in NewManifests.
Test Plan
In combination with expo/expo-cli#3606:
EXPO_DEBUG=true ~/expo/expo-cli/node_modules/.bin/expo start
in a project.