[asset] Support fromModule in Snack web environment #14435

IjzerenHein · 2021-09-16T13:53:02Z

Why

The Asset.fromModule method fails in the Snack environment due to various reasons. This PR makes some straightforward fixes to Asset.fromModule and adds some changes to support the Snack environment.

This is important to support examples such as ImageManipulator and all other examples that depend on Asset.fromModule on web.

How

Check for nativeExtensions var before using it (this is not available in the Snack env)
Clean up getScriptURL and _coerceLocalScriptURL which always returned null
Add support for Snack asset urls which have optional asset-types
Fix non-server urls which were always prefixed with file://

Test Plan

Tested within Snack web, see here: [runtime] Fix Asset.fromModule on web snack#214

IjzerenHein · 2021-09-16T14:17:29Z

packages/expo-asset/src/AssetSourceResolver.web.ts

@@ -37,7 +37,7 @@ function getScaledAssetPath(asset): string {
  const scale = AssetSourceResolver.pickScale(asset.scales, getScale());
  const scaleSuffix = scale === 1 ? '' : '@' + scale + 'x';
  const assetDir = getBasePath(asset);
-  return assetDir + '/' + asset.name + scaleSuffix + '.' + asset.type;
+  return assetDir + '/' + asset.name + scaleSuffix + (asset.type ? `.${asset.type}` : '');


This is needed to support Snack assets served from S3 (which don't have an extension)

so a trailing . crashes it? Or is the issue that we are trying to deal with some strange handling of null?

It's not crashing it, but it produces the wrong url. asset.type is optional and when omitted this code produces the wrong url-suffix: .undefined

for reference, we've also been patching react-native-web (for as long as I can remember) in snack to support this:
https://github.com/expo/snack/blob/main/runtime/patches/react-native-web%2B0.13.18.patch#L10

IjzerenHein · 2021-09-16T14:18:52Z

packages/expo-asset/src/resolveAssetSource.web.ts

-function _coerceLocalScriptURL(scriptURL: string | undefined | null): string | null {
-  if (scriptURL) {
-    if (scriptURL.startsWith('assets://')) {
-      // android: running from within assets, no offline path to use
-      return null;
-    }
-    scriptURL = scriptURL.substring(0, scriptURL.lastIndexOf('/') + 1);
-    if (!scriptURL.includes('://')) {
-      // Add file protocol in case we have an absolute file path and not a URL.
-      // This shouldn't really be necessary. scriptURL should be a URL.
-      scriptURL = 'file://' + scriptURL;
-    }
-  }
-  return null;
-}
-
-function getScriptURL(): string | null {
-  if (_scriptURL === undefined) {
-    _scriptURL = _coerceLocalScriptURL(getSourceCodeScriptURL());
-  }
-  return _scriptURL;
-}
-


_coerceLocalScriptURL always returns null causing getScriptURL to also always return null.
@EvanBacon Do you know whether this was ever intentional?

IjzerenHein · 2021-09-16T14:21:03Z

packages/expo-asset/src/AssetSourceResolver.web.ts

@@ -84,7 +84,7 @@ export default class AssetSourceResolver {
    return this.fromSource(getScaledAssetPath(this.asset));
  }
  scaledAssetURLNearBundle(): ResolvedAssetSource {
-    const path = this.jsbundleUrl || 'file://';


Prepending file:// to the web asset path actually breaks it. This is because getScaledAssetPath returns the httpServerLocation which already has a protocol prefix

jkhales

LGTM, but I'm also not super familiar with the web stuff.

jkhales · 2021-09-16T19:00:59Z

packages/expo-asset/src/resolveAssetSource.web.ts

 let _sourceCodeScriptURL: string | undefined | null;

 function getSourceCodeScriptURL(): string | undefined | null {
  if (_sourceCodeScriptURL) {
    return _sourceCodeScriptURL;
  }

-  let sourceCode = nativeExtensions && nativeExtensions.SourceCode;
+  let sourceCode = typeof nativeExtensions !== 'undefined' ? nativeExtensions.SourceCode : null;


Suggested change

let sourceCode = typeof nativeExtensions !== 'undefined' ? nativeExtensions.SourceCode : null;

let sourceCode = nativeExtensions?.SourceCode ?? null;

or did you want an undefined to leak through if SourceCode was undefined?

The suggestion would break stuff as nativeExtensions is not a declared global in the Snack environment. The PR makes the change to not depend on this nativeExtensions global

I don't understand the point about "declared global"?

seems to make logical sense and to pass ts analysis in vscode. I also read through some documentation: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html

Can you explain this typescript danger you are seeing a bit more so I can learn to not make the mistake in the future?

On native, nativeExtensions is injected by react-native into the global namespace. On web this global is however not available and produces the following runtime error, when accessed:

resolveAssetSource.web.ts:18 Uncaught (in promise) ReferenceError: nativeExtensions is not defined at u (resolveAssetSource.web.ts:18) at resolveAssetSource.web.ts:28 ...

When using nativeExtensions?.SourceCode, it would still try to access the global nativeExtensions variable and produce the runtime error. By using typeof nativeExtensions !== 'undefined' you can check whether the global exists before accessing it. Another way of doing that is by using global.nativeExtensions to check whether the global exists.

This is the same kind of error that you get when for instance accessing the window global from your server rendered page.

Anyway, many thanks for the review @jkhales 👍 👍

jkhales · 2021-09-16T19:05:39Z

packages/expo-asset/src/resolveAssetSource.web.ts

  }
-  _sourceCodeScriptURL = sourceCode.scriptURL;
+  _sourceCodeScriptURL = sourceCode?.scriptURL;


Suggested change

_sourceCodeScriptURL = sourceCode?.scriptURL;

return (nativeExtensions?.SourceCode ?? NativeModules?.SourceCode)?.scriptURL

Would let you get rid of 5 lines

This would produce the same runtime error and also no longer cache the _sourceCodeScriptURL for subsequent calls

* [asset] Fix fromModule exception on web * [asset] Support Snack web assets * [asset] Support Snack web assets * update build files * fix * update changelog * Add nativeExtension typing * Update build files

These changes ship with expo-asset by default now expo/expo#14435

* [runtime] Upgrade to Expo SDK 43 stable * [runtime] Remove expo-asset patch These changes ship with expo-asset by default now expo/expo#14435 * [runtime] Update web customization files with cli * [snack-sdk] Remove bundled native modules for SDK 39 * [snack-sdk] Update bundled native modules for sdk 43 * [snack-sdk] Drop Expo SDK 39 from sdk * [snack-sdk] Update tests and snapshots * [snack-sdk] Add changelog message about sdk 43 upgrade * [website] Update the visible SDK versions * [website] Update tests and snapshots * [snackager] Update tests and snapshots * [snackager] Set bundler test timeout to 2 minutes * [ci] Force integration tests to run in band without blocking fast tests

# Why - fix expo/router#131 by using the correct web base URL, previously was failing to resolve the origin using a number of naive checks. - drop all incorrect offline checks for web assets - utilize the newer @react-native/assets/registry package - reuse upstream types - fix file system type error - Possibly breaks web snack support introduced here #14435 - ~~Also might break some assumptions about Expo Webpack, will need to do some testing.~~ Webpack just ignores all of this code.  # How  # Test Plan  # Checklist  - [ ] Documentation is up to date to reflect these changes (eg: https://docs.expo.dev and README.md). - [ ] Conforms with the [Documentation Writing Style Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md) - [ ] This diff will work correctly for `expo prebuild` & EAS Build (eg: updated a module plugin). Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>

expo-bot added the bot: suggestions ExpoBot has some suggestions label Sep 16, 2021

IjzerenHein mentioned this pull request Sep 16, 2021

[runtime] Fix Asset.fromModule on web expo/snack#214

Merged

IjzerenHein commented Sep 16, 2021

View reviewed changes

IjzerenHein marked this pull request as ready for review September 16, 2021 14:21

IjzerenHein requested a review from tsapeta as a code owner September 16, 2021 14:21

IjzerenHein requested review from EvanBacon and jkhales September 16, 2021 14:21

jkhales approved these changes Sep 16, 2021

View reviewed changes

IjzerenHein added 8 commits October 14, 2021 09:08

[asset] Fix fromModule exception on web

e5216b4

[asset] Support Snack web assets

d1bde7f

[asset] Support Snack web assets

6533b0c

update build files

0b17096

fix

d8cf32c

update changelog

0f7be97

Add nativeExtension typing

afe7944

Update build files

c7d9eec

IjzerenHein force-pushed the @hein/asset/from-module-snack-web branch from eeda53f to c7d9eec Compare October 14, 2021 07:10

expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Oct 14, 2021

IjzerenHein merged commit 7049ee5 into master Oct 21, 2021

IjzerenHein deleted the @hein/asset/from-module-snack-web branch October 21, 2021 11:21

byCedric added a commit to expo/snack that referenced this pull request Oct 28, 2021

[runtime] Remove expo-asset patch

e37e213

These changes ship with expo-asset by default now expo/expo#14435

EvanBacon mentioned this pull request Nov 29, 2022

chore(expo-asset): remove unused web features #20258

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[asset] Support fromModule in Snack web environment #14435

[asset] Support fromModule in Snack web environment #14435

IjzerenHein commented Sep 16, 2021 •

edited

Loading

IjzerenHein Sep 16, 2021

jkhales Sep 16, 2021

IjzerenHein Oct 7, 2021

IjzerenHein Sep 16, 2021

IjzerenHein Sep 16, 2021

jkhales left a comment

jkhales Sep 16, 2021

IjzerenHein Oct 7, 2021

IjzerenHein Oct 7, 2021

jkhales Oct 7, 2021

IjzerenHein Oct 14, 2021 •

edited

Loading

IjzerenHein Oct 14, 2021

jkhales Sep 16, 2021

IjzerenHein Oct 14, 2021

	let sourceCode = typeof nativeExtensions !== 'undefined' ? nativeExtensions.SourceCode : null;
	let sourceCode = nativeExtensions?.SourceCode ?? null;

	_sourceCodeScriptURL = sourceCode?.scriptURL;
	return (nativeExtensions?.SourceCode ?? NativeModules?.SourceCode)?.scriptURL

[asset] Support fromModule in Snack web environment #14435

[asset] Support fromModule in Snack web environment #14435

Conversation

IjzerenHein commented Sep 16, 2021 • edited Loading

Why

How

Test Plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkhales left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IjzerenHein Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IjzerenHein commented Sep 16, 2021 •

edited

Loading

IjzerenHein Oct 14, 2021 •

edited

Loading