[metro-config] Remove json assetExt

[jonsamp]

Why

When we create update bundles, metro includes all assets necessary to run the app in its output. Part of our metro resolver configuration includes assetExts and sourceExts. assetExts are file extensions that metro will include as discreet assets alongside the JS bundles. sourceExts are file extensions that metro will include inside the JS bundle, inline.

I noticed that we have json in both assetExts and sourceExts, which means we are inlining json files in the main JS bundle and then including them again as discreet assets. In other words, all json files in update bundles are duplicated.

With EAS Update, developers often have node modules that include many json files (like translations etc), which we then count against our 400 asset limit. This can make it hard for larger apps to publish. Since those apps do not need the extra json assets because they are also inlined, this PR removes json from assetExts.

Below I ran expod export --experimental-bundle on a project:

Before

saving /assets/fonts/SpaceMono-Regular.ttf
saving /node_modules/@react-navigation/elements/src/assets/back-icon@1x.android.png
saving /node_modules/@react-navigation/elements/src/assets/back-icon@1.5x.android.png
saving /node_modules/@react-navigation/elements/src/assets/back-icon@2x.android.png
saving /node_modules/@react-navigation/elements/src/assets/back-icon@3x.android.png
saving /node_modules/@react-navigation/elements/src/assets/back-icon@4x.android.png
saving /node_modules/@react-navigation/elements/src/assets/back-icon-mask.png
saving /node_modules/react-native/package.json
saving /node_modules/mdn-data/css/at-rules.json
saving /node_modules/mdn-data/css/properties.json
saving /node_modules/mdn-data/css/syntaxes.json
saving /node_modules/css-tree/data/patch.json
saving /node_modules/css-tree/package.json
saving /node_modules/entities/lib/maps/entities.json
saving /node_modules/entities/lib/maps/legacy.json
saving /node_modules/entities/lib/maps/xml.json
saving /node_modules/entities/lib/maps/decode.json
saving /node_modules/dom-serializer/foreignNames.json
saving /node_modules/css-select/lib/procedure.json
saving /assets/databases/CSW21.db
saving /assets/databases/NWL2020.db
saving /tailwind.json
saving /node_modules/@react-navigation/elements/src/assets/back-icon@1x.ios.png
saving /node_modules/@react-navigation/elements/src/assets/back-icon@1.5x.ios.png
saving /node_modules/@react-navigation/elements/src/assets/back-icon@2x.ios.png
saving /node_modules/@react-navigation/elements/src/assets/back-icon@3x.ios.png
saving /node_modules/@react-navigation/elements/src/assets/back-icon@4x.ios.png

After

saving /assets/fonts/SpaceMono-Regular.ttf
saving /node_modules/@react-navigation/elements/src/assets/back-icon@1x.android.png
saving /node_modules/@react-navigation/elements/src/assets/back-icon@1.5x.android.png
saving /node_modules/@react-navigation/elements/src/assets/back-icon@2x.android.png
saving /node_modules/@react-navigation/elements/src/assets/back-icon@3x.android.png
saving /node_modules/@react-navigation/elements/src/assets/back-icon@4x.android.png
saving /node_modules/@react-navigation/elements/src/assets/back-icon-mask.png
saving /assets/databases/CSW21.db
saving /assets/databases/NWL2020.db
saving /node_modules/@react-navigation/elements/src/assets/back-icon@1x.ios.png
saving /node_modules/@react-navigation/elements/src/assets/back-icon@1.5x.ios.png
saving /node_modules/@react-navigation/elements/src/assets/back-icon@2x.ios.png
saving /node_modules/@react-navigation/elements/src/assets/back-icon@3x.ios.png
saving /node_modules/@react-navigation/elements/src/assets/back-icon@4x.ios.png

How

• In getDefaultConfig(), I filtered out json from the default assetExts, then added tests.

Test Plan

This change affects development and production bundles. To see the result of this change, pull this branch, then run yarn link in expo-cli/packages/metro-config. Then, in an Expo project run yarn link @expo/metro-config. Back in expo-cli, run yarn start to build the packages.

Back in the Expo project, you can run expod export --experimental-bundle (I have expod as an alias: alias expod="~/<your-file-path>/expo-cli/packages/expo-cli/bin/expo.js").

After running this, you should see that json files are not emitted as part of the bundle.

Also:

• Make sure that you can run expod start and see changes
• Make sure you can run expod publish and eas update. Make sure there are no json files in the metadata.json file.
• Make sure that when supplying a custom metro.config.json in a project, that if you add json in assetExts, that it does include json files in the outputted bundle as assets.

[ide]

Is this a breaking change, albeit a good one? Is there ever a scenario where including "json" in both sourceExts and assetExts behaves differently than if it were just in sourceExts? All imports/requires for JSON files presumably would have returned the bundled JSON, not the uploaded-as-an-asset JSON.

I think we should document this as a breaking change regardless in case there are edge cases we haven't anticipated, but outside of contrived scenarios I imagine this works well for 99% of apps.

[EvanBacon]

From Christoph Nakazawa in the Metro discord:

You'll have to add them explicitly as assetExts
There is some confusion in Metro where some files can be both source and asset files (JSON is one such file). This is very confusing and RN (at least at FB) depends on both behaviors that JSON files can be accessed from native via a request to Metro as well as from JS as a regular module.

I think changing how JSON is loaded is a pretty major change, I'm not sure how to proceed.

[ide]

My understanding of the situation is that it's undesirable for apps to include JSON twice: obviously we don't want each JSON file to be both bundled and included as a standalone asset at the same time. However, it sounds like there are also some edge cases that we might not anticipate.

What if we made it so that by default, we excluded JSON from assetExts, but allowed developers to add it back explicitly the same way they'd add any other assetExt? This way the change in behavior would be a breaking change with a straightforward way to restore the old behavior by opting into it: high ceilings with good defaults.

[kbrandwijk]

My understanding of the situation is that it's undesirable for apps to include JSON twice: obviously we don't want each JSON file to be both bundled and included as a standalone asset at the same time. However, it sounds like there are also some edge cases that we might not anticipate.

What if we made it so that by default, we excluded JSON from assetExts, but allowed developers to add it back explicitly the same way they'd add any other assetExt? This way the change in behavior would be a breaking change with a straightforward way to restore the old behavior by opting into it: high ceilings with good defaults.

By that reasoning, can't the user already filter json from either assetExts or sourceExts themselves in their project metro config, depending on their needs and desired behavior, without this change to the defaults? Because if so, is changing the default to something that also is not the default for RN worth the risk of introducing a breaking change to an unknown subset of existing users?

[ide]

The principle we're striving for is high ceilings with good defaults and the current default isn't fundamentally good. From Evan's comment it sounds like there are some cases where accidental complexity relies on duplicate JSON files and while we want to allow that if an app wants to take on that cost, I imagine most apps don't need it.

[jonsamp]

Following up here. A summary:

In either case, developers can specify which assetExts they'd like to include or not include in their bundle. So the question is which should be the default.

Removing json:

• Downside: This could have unknown consequences, as Christoph pointed out.
  • Are we able to specify what exactly might break? I prefer not to make code decisions out of fear of the unknown. If we can't really know, then this should be less of an issue. In the worst-case scenario, developers would not be able to develop or publish their project. In that case, we'd roll this back, and instruct developers to add json to their assetExts or to roll back to a previous Expo CLI version. This worst-case scenario would lower our trust with developers to some extent.
• Upside: This would include fewer assets in each update, which is a win for Expo, developers, and end-users. Updates will be faster. It's also a good default that developers would not have to think about, and more projects will be able to publish with EAS Update.

Keeping json (closing this PR):

• Downside: Updates are slower than they could be. We likely need to instruct developers to remove json in a custom metro config to make their updates as fast as possible. That extra step is not great.
• Upside: We avoid any of the worst-case scenarios above.

I tested this on 3 of my apps, however, I can't possibly test on a wide enough array of projects to know for certain that this will be safe. To me, the upsides outweigh the downsides/risk. The worst-case scenario seems like it's relatively easy to address.

Would love to hear your thoughts on this + any other upsides/downsides.

[ide]

We'll need to communicate these clearly in the next SDK's release notes but this looks good. I would expect it to reduce update sizes significantly.