[expo-updates] rename Update.metadata -> Update.manifest #12818

esamelson · 2021-05-04T23:54:33Z

Why

How

Android: use Android Studio to rename the property on UpdateEntity
iOS: search + replace all usages of update.metadata and metadata: with update.manifest and manifest:; manually modify a few more places in EXUpdatesUpdate.h/.m. Project-wide search of metadata to ensure no other relevant usages were missed.

I also changed the logic for embedded (bare) updates slightly; previously we looked for a field in the manifest called metadata and stored that in the db column, but we never actually had that field, so I've just removed that logic entirely.

To be clear, this doesn't change anything about current behavior; we store no manifest for embedded updates both before and after this PR. But it does open up the question -- should we be storing the embedded/bare manifest for embedded updates? My thinking is no, because it is so different from the format of both legacy and new updates, that manifest consumers (dev client, any developer using Constants.manifest/Updates.manifest, etc.) are better served handling a null/empty case than explicitly having to handle multiple possible formats. And the manifest for any embedded update is of no use unless it's the update that's embedded in the currently installed build -- in which case the manifest is available on disk. Open to other thoughts though!

Test Plan

All unit tests pass; also ran Expo Go builds on each platform and verified that the versioned Updates module methods work. ✅

Checklist

Documentation is up to date to reflect these changes (eg: https://docs.expo.io and README.md).
This diff will work correctly for expo build (eg: updated @expo/xdl).
This diff will work correctly for expo prebuild & EAS Build (eg: updated a module plugin).

expo-bot

The review previously left here is no longer valid, jump to the latest one 👉 #12818 (review)

ide · 2021-05-05T02:31:19Z

It looks like the code that looked for a field named "metadata" was a mistake and should have been looking for "updateMetadata". Unless we want to intentionally exclude the metadata field from embedded manifests if we were to define metadata, shouldn't we add back the code that copies manifest.metadata and fix -[EXUpdatesBareRawManifest metadata]?

@jkhales Relatedly, was the name "updateMetadata" chosen intentionally over "metadata"? Setting aside the way the code happens to be written right now, would "metadata" be a more appropriate field to use in manifests? This is the easiest it will be from here on out to make this change.

expo-bot

The review previously left here is no longer valid, jump to the latest one 👉 #12818 (review)

esamelson · 2021-05-05T21:07:51Z

This is a little bit tangled because in my original database schema design, this column was intended for a field named metadata in the manifest, but in the actual implementation it quickly evolved into a place to store the entire manifest (hence the rename in #12768 and here).

The bare manifest was the only type of manifest still storing the value of a metadata field in this column, rather than the entire manifest -- but no such field has ever existed in bare manifests up to now, which is why I removed that logic entirely in this PR.

Reading @ide 's comment, I think there are three separate questions to resolve here:

Do we want a separate column besides manifest in which to store some metadata (whether the actual field name in the manifest is updateMetadata, metadata, or something else)? Or is it sufficient to simply store the whole manifest and get the metadata from that JSON object at runtime when needed?
What do we want the actual field name in {new, embedded} manifests to be -- updateMetadata, metadata, or something else?
For bare manifests, do we want to store anything in the newly-renamed manifest field, or continue leaving it empty?

My current opinions are:

I don't see any value to a separate column because I don't think we'll ever do any database-level operations with this field. One benefit it would provide is letting update consumers be agnostic of the metadata's field name in the manifest, but we get this same benefit from Will's strictly typed manifest classes.
No strong opinion, will defer to @jkhales -- but agree with James that now is the best time to change it.
I think leaving it empty is best, because (a) it's not useful unless it's for the currently embedded update, in which case the manifest is already available from disk and (b) forcing consumers of this field to handle null is better than forcing them to handle yet a third different manifest format (plus makes it harder for us to change this format at will).

ide · 2021-05-05T23:02:26Z

I agree that it's not worth a separate column. The hypothetical metadata column would likely be an unstructured JSON column, giving us no additional database integrity constraints over the manifest column. And the marginal disk usage is trivial in this context.
I like metadata in the sense that all the other fields in the manifest describe the update and we don't write updateAssets, updateId, etc...
Agree with empty, specifically null and not {}. I think the way to reconcile this in the future is to make embedded manifests include all of the fields of downloaded manifests, at least in contexts where that's possible. (For example, if we have a build step that figures out the EAS Update URL from the EAS project ID, that same step could perhaps query enough info from EAS to generate an embedded manifest that sufficiently overlaps with a downloaded one.)

jkhales

Put up PR to change updateMetadata -> metadata

https://github.com/expo/universe/pull/7512

esamelson · 2021-05-06T00:40:44Z

OK, sounds like we're on the same page. These answers for (1) and (3) are what this PR implements for bare manifests, so I'll land it once CI finishes.

Re: changing updateMetadata to metadata in the new manifest spec -- seems like a reasonable change. I think it should happen in a separate PR than this one. @jkhales would you be down to take care of this change client-side as well? I think we'll need to change the typescript type in #12817 along with the native implementation.

And re: making the embedded manifest format closer to a downloaded one -- we could do that, though there are a number of differences that have to be there. For example, asset objects in embedded manifests may not have a url, and they instead contain some other fields with info about how the asset is stored in the package/NSBundle. We could create a supertype that includes all these possibilities, but IMO it would be nice to keep the embedded manifest type as private as possible so that we can more easily iterate on it as needed (e.g. if we start storing iOS assets using the asset catalog).

ide · 2021-05-06T00:44:33Z

For example, asset objects in embedded manifests may not have a url, and they instead contain some other fields with info about how the asset is stored in the package/NSBundle.

I think the answer to this would be to support non-HTTP URLs like file:.

expo-bot

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 20821f5

ide · 2021-05-06T00:45:41Z

packages/expo-updates/android/src/main/java/expo/modules/updates/manifest/BareManifest.kt

@@ -19,7 +19,6 @@ class BareManifest private constructor(
  private val mScopeKey: String,
  private val mCommitTime: Date,
  private val mRuntimeVersion: String,
-  private val mMetadata: JSONObject?,


Since we do want to copy the metadata over eventually, what do you think about reverting these changes?

Based on the discussion above, I don't think we ever want to store just the manifest's (theoretical) metadata field in the database separately from the raw manifest, which is already its own separate field in this class (and constructor). Maybe I'm misunderstanding what you're asking though? 🤔

Okay, I see. I was thinking we might want BareManifest.metadata to be defined or for there to be a way to get the metadata directly from an UpdateEntity. But it looks like we don't need either of those.

esamelson · 2021-05-06T00:55:41Z

I think the answer to this would be to support non-HTTP URLs like file:.

In order to do that, we need to come up with a URL scheme/protocol for files stored in res on Android, which includes the res folder, filename, and list of supported scales, so we can destructure it in native code. That's definitely doable though I think it's cleaner to have the individual fields.

jkhales · 2021-05-06T01:42:39Z

Re: changing updateMetadata to metadata in the new manifest spec -- seems like a reasonable change. I think it should happen in a separate PR than this one. @jkhales would you be down to take care of this change client-side as well? I think we'll need to change the typescript type in #12817 along with the native implementation.

Yup, had already started on it. tbh probably should have waited to finish it before merging the www change. But doesn't seem too complex. Will try to put a PR first thing tomorrow.

[edit] stacked a PR on top of this one. Will rebase and ask for your review once you merge this one. #12831

ide · 2021-05-06T02:27:08Z

packages/expo-updates/android/src/main/java/expo/modules/updates/manifest/BareManifest.kt

@@ -19,7 +19,6 @@ class BareManifest private constructor(
  private val mScopeKey: String,
  private val mCommitTime: Date,
  private val mRuntimeVersion: String,
-  private val mMetadata: JSONObject?,


Okay, I see. I was thinking we might want BareManifest.metadata to be defined or for there to be a way to get the metadata directly from an UpdateEntity. But it looks like we don't need either of those.

esamelson requested review from ide and jkhales May 4, 2021 23:54

expo-bot reviewed May 4, 2021

View reviewed changes

Base automatically changed from @eric/last-accessed-3 to master May 5, 2021 19:34

esamelson force-pushed the @eric/last-accessed-3.5 branch from cdf99b6 to a78eb4c Compare May 5, 2021 19:34

expo-bot self-requested a review May 5, 2021 19:37

expo-bot reviewed May 5, 2021

View reviewed changes

jkhales approved these changes May 6, 2021

View reviewed changes

esamelson added 3 commits May 5, 2021 17:42

[android] rename UpdateEntity.metadata -> manifest

ca61fcf

[ios] rename EXUpdatesUpdate.metadata -> manifest

c626f21

update changelog

20821f5

esamelson force-pushed the @eric/last-accessed-3.5 branch from a78eb4c to 20821f5 Compare May 6, 2021 00:43

expo-bot self-requested a review May 6, 2021 00:45

expo-bot reviewed May 6, 2021

View reviewed changes

ide reviewed May 6, 2021

View reviewed changes

jkhales mentioned this pull request May 6, 2021

[expo-updates][ios][android] rename updateMetadata to metadata #12831

Merged

ide approved these changes May 6, 2021

View reviewed changes

esamelson merged commit 282a913 into master May 6, 2021

esamelson deleted the @eric/last-accessed-3.5 branch May 6, 2021 03:17

tsapeta pushed a commit that referenced this pull request May 7, 2021

[expo-updates] rename Update.metadata -> Update.manifest (#12818)

241de8d

wschurman mentioned this pull request Jun 27, 2023

[expo-updates][ios][49-only] Make manifest nil for embedded bare updates #23147

Merged

3 tasks

linear bot mentioned this pull request Aug 21, 2023

[expo-constants] Change source of truth for constants types #24049

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[expo-updates] rename Update.metadata -> Update.manifest #12818

[expo-updates] rename Update.metadata -> Update.manifest #12818

esamelson commented May 4, 2021

expo-bot left a comment •

edited

Loading

ide commented May 5, 2021

expo-bot left a comment •

edited

Loading

esamelson commented May 5, 2021 •

edited

Loading

ide commented May 5, 2021

jkhales left a comment

esamelson commented May 6, 2021 •

edited

Loading

ide commented May 6, 2021

expo-bot left a comment

ide May 6, 2021

esamelson May 6, 2021 •

edited

Loading

ide May 6, 2021

esamelson commented May 6, 2021

jkhales commented May 6, 2021 •

edited

Loading

ide May 6, 2021

[expo-updates] rename Update.metadata -> Update.manifest #12818

[expo-updates] rename Update.metadata -> Update.manifest #12818

Conversation

esamelson commented May 4, 2021

Why

How

Test Plan

Checklist

expo-bot left a comment • edited Loading

Choose a reason for hiding this comment

ide commented May 5, 2021

expo-bot left a comment • edited Loading

Choose a reason for hiding this comment

esamelson commented May 5, 2021 • edited Loading

ide commented May 5, 2021

jkhales left a comment

Choose a reason for hiding this comment

esamelson commented May 6, 2021 • edited Loading

ide commented May 6, 2021

expo-bot left a comment

Choose a reason for hiding this comment

ide May 6, 2021

Choose a reason for hiding this comment

esamelson May 6, 2021 • edited Loading

Choose a reason for hiding this comment

ide May 6, 2021

Choose a reason for hiding this comment

esamelson commented May 6, 2021

jkhales commented May 6, 2021 • edited Loading

ide May 6, 2021

Choose a reason for hiding this comment

expo-bot left a comment •

edited

Loading

expo-bot left a comment •

edited

Loading

esamelson commented May 5, 2021 •

edited

Loading

esamelson commented May 6, 2021 •

edited

Loading

esamelson May 6, 2021 •

edited

Loading

jkhales commented May 6, 2021 •

edited

Loading