Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upstream changes to resolveAssetSource.js #10619

Open
rozele opened this issue Sep 22, 2022 · 8 comments
Open

Upstream changes to resolveAssetSource.js #10619

rozele opened this issue Sep 22, 2022 · 8 comments
Labels
Area: Fabric Support Facebook Fabric Area: Image enhancement Needs: Dev Design New Architecture Broad category for issues that apply to the RN "new" architecture of Turbo Modules + Fabric Partner: Facebook Workstream: Releases and Integrations Keep RNW current with RN releases.
Milestone

Comments

@rozele
Copy link
Collaborator

rozele commented Sep 22, 2022

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:<path>" 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

@ghost ghost added Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) Partner: Facebook labels Sep 22, 2022
@chrisglein chrisglein added Needs: Dev Design and removed Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) labels Sep 26, 2022
@chrisglein chrisglein added this to the 0.71 milestone Sep 26, 2022
@chrisglein
Copy link
Member

Needed for the workaround for #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
Copy link
Member

Related: #10620

@rozele
Copy link
Collaborator Author

rozele commented Sep 26, 2022

Needed for the workaround for #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 chiaramooney modified the milestones: 0.71, 0.72 Jan 9, 2023
@chiaramooney chiaramooney added the Needs: Attention 👋 An issue that had been tagged "Needs: Author Feedback" has received activity (label applied by bot) label Jan 9, 2023
@chiaramooney
Copy link
Contributor

Flagging to discuss. Issue has milestone but no assignee.

@chrisglein chrisglein modified the milestones: 0.72, Backlog Jan 19, 2023
@chrisglein chrisglein removed the Needs: Attention 👋 An issue that had been tagged "Needs: Author Feedback" has received activity (label applied by bot) label Jan 19, 2023
@chrisglein
Copy link
Member

chrisglein commented Jan 19, 2023

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
Copy link
Collaborator Author

rozele commented Jan 23, 2023

@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
Copy link
Collaborator

jonthysell commented Jan 25, 2023

@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
Copy link
Collaborator Author

rozele commented Jan 25, 2023

@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.

@jonthysell jonthysell added the New Architecture Broad category for issues that apply to the RN "new" architecture of Turbo Modules + Fabric label Mar 7, 2024
@chiaramooney chiaramooney added the Workstream: Releases and Integrations Keep RNW current with RN releases. label Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Fabric Support Facebook Fabric Area: Image enhancement Needs: Dev Design New Architecture Broad category for issues that apply to the RN "new" architecture of Turbo Modules + Fabric Partner: Facebook Workstream: Releases and Integrations Keep RNW current with RN releases.
Projects
Status: No status
Development

No branches or pull requests

5 participants