microsoft / react-native-windows

A framework for building native Windows apps with React.
https://microsoft.github.io/react-native-windows/
Other
16.28k stars 1.14k forks source link

Upstream changes to resolveAssetSource.js #10619

Open rozele opened 2 years ago

rozele commented 2 years ago

Summary

We need to override Libraries/Image/resolveAssetSource.js for unpackaged apps. There are assumptions in the upstream version of resolveAssetSource.js that the root bundle path is of the form <protocol>://<path>, and unpackaged apps may use bundle root paths of the form "C:\" or otherwise.

Motivation

We ship both packaged and unpackaged versions of our app. We should support both scenarios.

Currently, react-native-windows has a bug where the source code URL for bundled apps is an empty string. We'll be fixing this so that apps can reliably use the Image.resolveAssetSource API from React Native to get a fully qualified path to a packaged asset.

In order for this bug fix to work with unpackaged apps, we need eliminate the assumption that bundle paths match the form <protocol>://<path>.

Basic Example

For example, in our app, we have a context menu native module that takes in an object including the URI for the Icon to use for a context menu item. It should be possible to use the Image.resolveAssetPath API to resolve the fully qualified asset path for the icon.

E.g.,

const icon = require('icon.png');
const iconSource = Image.resolveAssetSource(icon);
NativeModules.ContextMenu.show({text: 'Item 1', icon: iconSource});

Without this change, unpackaged app icon sources will always be resolved to file://icon.png and we need native logic in the context menu module to strip the file:// prefix and prepend the bundle root path from the instance settings snapshot.

I'd prefer that we have common logic that is reusable for all modules that works in roughly the same way resolveAssetSource behaves on other platforms.

Open Questions

No response

chrisglein commented 2 years ago

Needed for the workaround for https://github.com/microsoft/react-native-windows/issues/10614?

There is logic for bundled apps to resolve these. Is this just needed for unbundled apps? Sounds like there's some offline discussion about how may want to retire the current file:// resolution.

chrisglein commented 2 years ago

Related: https://github.com/microsoft/react-native-windows/issues/10620

rozele commented 2 years ago

Needed for the workaround for https://github.com/microsoft/react-native-windows/issues/10614?

No, workaround is for #10620>

Sounds like there's some offline discussion about how may want to retire the current file:// resolution.

What's the offline discussion? I don't think there has ever been file:// resolution, there was only a bug caused by the missing line in SourceCodeModule to resolve the base path for the JS bundle in release builds.

chiaramooney commented 1 year ago

Flagging to discuss. Issue has milestone but no assignee.

chrisglein commented 1 year ago

Need to look at this issue when we look more closely at the image pipeline for Fabric.

https://github.com/microsoft/react-native-windows/blob/main/vnext/Microsoft.ReactNative/Views/Image/ImageViewManager.cpp#L186-L192

@rozele We spent some time discussing this in triage and it's clearly deeper than expected at first glance. Shall we cover in one of the Wednesday meetings?

rozele commented 1 year ago

@rozele We spent some time discussing this in triage and it's clearly deeper than expected at first glance. Shall we cover in one of the Wednesday meetings?

I'll be sure to add it to the agenda for this weeks meeting. cc @stmoy in case I forget 😅

jonthysell commented 1 year ago

@rozele ~Does your PR #10621 resolve this issue as well?~ Nevermind, I see now. So the change for this bug would be to make sure that unpackaged apps get a fully resolved file:// path?

rozele commented 1 year ago

@jonthysell The fix in #10621 would change behavior in both packaged and unpackaged apps. Previously, the SourceCodeModule would return empty string for "bundled" apps (i.e., those not loaded from the packager server). Now, SourceCodeModule will return the directory that the bundle was loaded from (which is what it does on iOS and Android). For packaged apps, the bundle path is generally something like ms-appx:///index.bundle, so the bundle root path would just be ms-appx:/// and all images would still be resolved with relative paths (e.g., ms-appx:///relative/path/to/image.png). Ultimately, this doesn't change the actual image resolution logic. Previously ImageViewManager had logic in it to automatically re-write file:// URIs to use the bundle root path from the ReactSettingsSnapshot. The value returned by SourceCodeModule is identical across packaged and unpackaged apps to the value in ReactSettingsSnapshot, so now the logic is just moving to resolveAssetSource (here).

It also makes it so that image source URL rewrite logic is the same code path across dev and prod (i.e., via JavaScript / resolveAssetSource), which is probably a good thing / may lead to fewer bugs.