Fixes issues with packaged asset resolution #10621

rozele · 2022-09-22T14:43:50Z

Description

Type of Change

Erase all that don't apply.

Bug fix (non-breaking change which fixes an issue)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Why

Image.resolveAssetSource has two different code paths - one for resolving assets from the packager server when running in standard debug builds and one for resolving assets when the JS package is pre-bundled.

Previously, we were not providing the bundle root path to the SourceCodeModule for pre-bundled apps, and it was up to each individual native module and view manager that depends on resolving asset paths with Image.resolveAssetSource to replace file:// with the bundle root path pulled from the InstanceSettingsSnapshot.

Even after fixing the issue with SourceCodeModule, there is still one more issue for unpackaged apps, whose asset paths may be Windows file paths (e.g., C:\...) rather than packaged asset paths (e.g., ms-appx://).

Resolves #10620

What

This change ensures that SourceCodeModule provides the bundle root path as the scriptURL constant and patches the bug in the upstream resolveAssetSource module to skip coersion of the bundle root path to a form like file://.

Please note, this is technically a "breaking change", any 3rd party native modules or view managers that leveraged Image.resolveAssetSource assuming it would return a file:// path with a relative asset path will now see that the path is already fully qualified with the bundle root path.

Testing

Created bundled version of Playground, ran RNTester, and confirmed packaged image assets still resolve correctly (in this case, the thumbs up image is a package asset):

Microsoft Reviewers: Open in CodeFlow

rozele · 2022-09-22T15:01:11Z

Please note, I've labeled this as, strictly speaking, this will change the contract of Image.resolveAssetSource, but assuming any 3rd party modules are doing appropriate URI validation, it's unlikely for anything to break.

asklar · 2022-09-22T15:29:50Z

vnext/src/Libraries/Image/resolveAssetSource.windows.js

+}
+
+function getScriptURL(): ?string {
+  if (_scriptURL === undefined) {


I can't see where _scriptURL comes from, but under which conditions would it be undefined? note that for image urls we support several non x-plat schemes apart from http/https, like ms-appx, ms-resource, resource, and some others; want to make sure this doesn't force these urls to be only "http/https" or "file" schemed

It's just memoizing a result.

This change should be independent of the actual bundle root path, whether it's ms-appx://, ms-resource:// or C:\... for unpackaged apps.

Here is where the memoization variable is declared: https://github.com/microsoft/react-native-windows/pull/10621/files/41a3c9a59bf619cbd37e6eae96c607cea1ef4552#diff-7ef60247116c017061f149892f29f5977c15debf02f5bac3038ba60e7c85b99aR22

vnext/src/Libraries/Image/resolveAssetSource.windows.js

asklar · 2022-09-22T15:34:27Z

vnext/Microsoft.ReactNative/Fabric/ImageComponentView.cpp

-      if (/*packager_assert && */ ris.uri.find("file://") == 0) {
-        ris.uri.replace(0, 7, ris.bundleRootPath);
-      }


I think the idea is that if you say file://foo.png, this turns into ms-appx:///Assets/Bundle/foo.png

Correct, but now with the change to Image.resolveAssetSource, the URI will already come in as ms-appx:///Assets/Bundle/foo.png

If you are truly consuming a bundled asset in React Native, you should do so by using:

<Image source={require('foo.png')} />

Rather than relying on an obscure implementation detail like:

<Image source={{uri: 'file://foo.png'}} />

So, this is another surface that is a "breaking change", but to be fair, whoever was depending on this implementation detail that they should not have been depending on it.

I'm not sure whether our internal partners are aware of this / using the right format, TBH. @stmoy FYI.

Actually, scratch that, using file://asset would never work because the resolved asset source would not include the internal '__packager_asset' flag, so this probably isn't breaking on that front.

It would only affect callsites where you use Image.resolveAssetPath and pass the result to some custom module or view manager that does not do proper validation on the URI (assumes that URIs will start with file://).

We needed zero code changes in Messenger when taking in this change. We were however able to delete a bunch of code for custom URI resolution.

Summary: `Image.resolveAssetSource` has two different code paths - one for resolving assets from the packager server when running in standard debug builds and one for resolving assets when the JS package is pre-bundled. Previously, we were not providing the bundle root path to the `SourceCodeModule` for pre-bundled apps, and it was up to each individual native module and view manager that depends on resolving asset paths with `Image.resolveAssetSource` to replace `file://` with the bundle root path pulled from the InstanceSettingsSnapshot. Even after fixing the issue with `SourceCodeModule`, there is still one more issue for unpackaged apps, whose asset paths may be Windows file paths (e.g., `C:\...`) rather than packaged asset paths (e.g., `ms-appx://`). This change ensures that SourceCodeModule provides the bundle root path as the `scriptURL` constant and patches the bug in the upstream `resolveAssetSource` module to skip coersion of the bundle root path to a form like `file://`. Resolves microsoft#10620 Test Plan: See test plan in D39735423 Reviewers: lyahdav, shawndempsey, chpurrer, mcenk, jacke, helenistic, mowang Reviewed By: mowang Differential Revision: https://phabricator.intern.facebook.com/D39732643 Tasks: T132565343, T131482365

acoates-ms · 2023-01-27T16:22:17Z

@rozele, looks like this PR breaks one of the integration tests.
I think this one isn't loading the image correctly for some reason.

react-native-windows/packages/integration-test-app/tests/SampleTests.tsx

Line 33 in f3475d4

source={require('react-native-windows/IntegrationTests/blue_square.png')}

Summary: `Image.resolveAssetSource` has two different code paths - one for resolving assets from the packager server when running in standard debug builds and one for resolving assets when the JS package is pre-bundled. Previously, we were not providing the bundle root path to the `SourceCodeModule` for pre-bundled apps, and it was up to each individual native module and view manager that depends on resolving asset paths with `Image.resolveAssetSource` to replace `file://` with the bundle root path pulled from the InstanceSettingsSnapshot. Even after fixing the issue with `SourceCodeModule`, there is still one more issue for unpackaged apps, whose asset paths may be Windows file paths (e.g., `C:\...`) rather than packaged asset paths (e.g., `ms-appx://`). This change ensures that SourceCodeModule provides the bundle root path as the `scriptURL` constant and patches the bug in the upstream `resolveAssetSource` module to skip coersion of the bundle root path to a form like `file://`. Resolves microsoft#10620 Test Plan: See test plan in D43159275 Reviewers: shawndempsey, chpurrer, lefever, helenistic Reviewed By: chpurrer Differential Revision: https://phabricator.intern.facebook.com/D43158676 Tasks: T140424672

Summary: Prior to PR microsoft#10621, the ImageViewManager for Paper and ImageViewComponent for Fabric required a workaround where file:// URIs were remapped to an absolute path using the InstanceSettings from the ReactContext. After microsoft#10621, the URI is already fully resolved by the time it reaches the native renderer for Image, so we do not need to do any URI rewrites. Test Plan: Images still load in RNTester: {F868525679} Reviewers: shawndempsey, helenistic, chpurrer, lefever Reviewed By: chpurrer Differential Revision: https://phabricator.intern.facebook.com/D43159275 Tasks: T140424672

chiaramooney · 2023-03-29T21:56:28Z

Waiting on merge conflict resolution and integration test to pass.

rozele · 2023-04-02T21:42:27Z

Thanks @chiaramooney - I'll rebase and take another look at the failing integration test.

rozele · 2023-06-02T14:53:09Z

@chiaramooney I think all the tests should pass now. The latest failure looks like a flaky build issue.

chiaramooney · 2023-06-07T20:48:16Z

/azp run

azure-pipelines · 2023-06-07T20:48:34Z

Azure Pipelines successfully started running 1 pipeline(s).

chiaramooney · 2023-07-05T22:48:52Z

/azp run

azure-pipelines · 2023-07-05T22:49:59Z

Azure Pipelines successfully started running 1 pipeline(s).

rozele · 2023-07-17T01:34:01Z

@chiaramooney I've had terrible luck running the integration tests locally, not sure how to fix this.

TatianaKapos · 2023-07-17T20:30:10Z

@rozele Would you mind rebasing off main one more time? Just to see if that will help :)

Summary: `Image.resolveAssetSource` has two different code paths - one for resolving assets from the packager server when running in standard debug builds and one for resolving assets when the JS package is pre-bundled. Previously, we were not providing the bundle root path to the `SourceCodeModule` for pre-bundled apps, and it was up to each individual native module and view manager that depends on resolving asset paths with `Image.resolveAssetSource` to replace `file://` with the bundle root path pulled from the InstanceSettingsSnapshot. Even after fixing the issue with `SourceCodeModule`, there is still one more issue for unpackaged apps, whose asset paths may be Windows file paths (e.g., `C:\...`) rather than packaged asset paths (e.g., `ms-appx://`). This change ensures that SourceCodeModule provides the bundle root path as the `scriptURL` constant and patches the bug in the upstream `resolveAssetSource` module to skip coersion of the bundle root path to a form like `file://`. Resolves microsoft#10620 Test Plan: See test plan in D43159275 Reviewers: shawndempsey, chpurrer, lefever, helenistic Reviewed By: chpurrer Differential Revision: https://phabricator.intern.facebook.com/D43158676 Tasks: T140424672

rozele · 2023-10-10T14:59:06Z

@rozele Would you mind rebasing off main one more time? Just to see if that will help :)

Rebased :)

`Image.resolveAssetSource` has two different code paths - one for resolving assets from the packager server when running in standard debug builds and one for resolving assets when the JS package is pre-bundled. Previously, we were not providing the bundle root path to the `SourceCodeModule` for pre-bundled apps, and it was up to each individual native module and view manager that depends on resolving asset paths with `Image.resolveAssetSource` to replace `file://` with the bundle root path pulled from the InstanceSettingsSnapshot. Even after fixing the issue with `SourceCodeModule`, there is still one more issue for unpackaged apps, whose asset paths may be Windows file paths (e.g., `C:\...`) rather than packaged asset paths (e.g., `ms-appx://`). This change ensures that SourceCodeModule provides the bundle root path as the `scriptURL` constant and patches the bug in the upstream `resolveAssetSource` module to skip coersion of the bundle root path to a form like `file://`. Resolves microsoft#10620

* Fixes issues with packaged asset resolution `Image.resolveAssetSource` has two different code paths - one for resolving assets from the packager server when running in standard debug builds and one for resolving assets when the JS package is pre-bundled. Previously, we were not providing the bundle root path to the `SourceCodeModule` for pre-bundled apps, and it was up to each individual native module and view manager that depends on resolving asset paths with `Image.resolveAssetSource` to replace `file://` with the bundle root path pulled from the InstanceSettingsSnapshot. Even after fixing the issue with `SourceCodeModule`, there is still one more issue for unpackaged apps, whose asset paths may be Windows file paths (e.g., `C:\...`) rather than packaged asset paths (e.g., `ms-appx://`). This change ensures that SourceCodeModule provides the bundle root path as the `scriptURL` constant and patches the bug in the upstream `resolveAssetSource` module to skip coersion of the bundle root path to a form like `file://`. Resolves microsoft#10620 * Change files

rozele requested review from a team as code owners September 22, 2022 14:43

rozele added the Breaking Change This PR will break existing apps and should be part of the known breaking changes for the release label Sep 22, 2022

rozele force-pushed the issue10620 branch 2 times, most recently from bc37366 to 41a3c9a Compare September 22, 2022 15:05

asklar reviewed Sep 22, 2022

View reviewed changes

vnext/src/Libraries/Image/resolveAssetSource.windows.js Outdated Show resolved Hide resolved

asklar reviewed Sep 22, 2022

View reviewed changes

rozele force-pushed the issue10620 branch from 6371a85 to c3c75a2 Compare October 11, 2022 03:11

chiaramooney requested a review from asklar November 2, 2022 23:33

chrisglein assigned asklar Dec 14, 2022

MacKenzieHnC mentioned this pull request Jan 7, 2023

Unable to access sqlite from react native windows #10431

Closed

jonthysell mentioned this pull request Jan 25, 2023

Upstream changes to resolveAssetSource.js #10619

Open

acoates-ms approved these changes Jan 26, 2023

View reviewed changes

asklar removed their assignment Mar 15, 2023

microsoft-github-policy-service bot added Area: Fabric Support Facebook Fabric Area: Image labels Mar 15, 2023

chiaramooney added the Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) label Mar 29, 2023

chrisglein assigned rozele Mar 29, 2023

microsoft-github-policy-service bot added Needs: Attention 👋 An issue that had been tagged "Needs: Author Feedback" has received activity (label applied by bot) and removed Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) labels Apr 2, 2023

rozele force-pushed the issue10620 branch 2 times, most recently from 75781e5 to 8950402 Compare April 2, 2023 23:42

rozele force-pushed the issue10620 branch from 8950402 to 232a826 Compare April 3, 2023 14:46

rozele force-pushed the issue10620 branch from 232a826 to af5ab69 Compare April 27, 2023 19:20

rozele force-pushed the issue10620 branch from af5ab69 to 98180e8 Compare June 2, 2023 13:29

rozele force-pushed the issue10620 branch from d75aee6 to 9fe8d1e Compare June 5, 2023 16:31

chiaramooney assigned TatianaKapos Jul 5, 2023

rozele force-pushed the issue10620 branch from 7f61604 to eba988b Compare October 10, 2023 14:55

rozele and others added 2 commits October 10, 2023 13:00

Change files

7027bfd

rozele force-pushed the issue10620 branch from 4ce0ec0 to 7027bfd Compare October 10, 2023 17:00

acoates-ms approved these changes Oct 14, 2023

View reviewed changes

acoates-ms merged commit e3c7f02 into microsoft:main Oct 14, 2023

jonthysell removed the Needs: Attention 👋 An issue that had been tagged "Needs: Author Feedback" has received activity (label applied by bot) label Oct 30, 2023

jonthysell added the New Architecture Broad category for issues that apply to the RN "new" architecture of Turbo Modules + Fabric label Mar 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes issues with packaged asset resolution #10621

Fixes issues with packaged asset resolution #10621

rozele commented Sep 22, 2022 •

edited

Loading

rozele commented Sep 22, 2022 •

edited

Loading

asklar Sep 22, 2022

rozele Sep 22, 2022

rozele Sep 22, 2022

asklar Sep 22, 2022 •

edited

Loading

rozele Sep 22, 2022

rozele Sep 22, 2022 •

edited

Loading

asklar Sep 22, 2022

rozele Sep 22, 2022

rozele Sep 22, 2022

acoates-ms commented Jan 27, 2023

chiaramooney commented Mar 29, 2023

rozele commented Apr 2, 2023

rozele commented Jun 2, 2023

chiaramooney commented Jun 7, 2023

azure-pipelines bot commented Jun 7, 2023

chiaramooney commented Jul 5, 2023

azure-pipelines bot commented Jul 5, 2023

rozele commented Jul 17, 2023

TatianaKapos commented Jul 17, 2023

rozele commented Oct 10, 2023

Fixes issues with packaged asset resolution #10621

Fixes issues with packaged asset resolution #10621

Conversation

rozele commented Sep 22, 2022 • edited Loading

Description

Type of Change

Why

What

Testing

Microsoft Reviewers: Open in CodeFlow

rozele commented Sep 22, 2022 • edited Loading

asklar Sep 22, 2022

Choose a reason for hiding this comment

rozele Sep 22, 2022

Choose a reason for hiding this comment

rozele Sep 22, 2022

Choose a reason for hiding this comment

asklar Sep 22, 2022 • edited Loading

Choose a reason for hiding this comment

rozele Sep 22, 2022

Choose a reason for hiding this comment

rozele Sep 22, 2022 • edited Loading

Choose a reason for hiding this comment

asklar Sep 22, 2022

Choose a reason for hiding this comment

rozele Sep 22, 2022

Choose a reason for hiding this comment

rozele Sep 22, 2022

Choose a reason for hiding this comment

acoates-ms commented Jan 27, 2023

chiaramooney commented Mar 29, 2023

rozele commented Apr 2, 2023

rozele commented Jun 2, 2023

chiaramooney commented Jun 7, 2023

azure-pipelines bot commented Jun 7, 2023

chiaramooney commented Jul 5, 2023

azure-pipelines bot commented Jul 5, 2023

rozele commented Jul 17, 2023

TatianaKapos commented Jul 17, 2023

rozele commented Oct 10, 2023

rozele commented Sep 22, 2022 •

edited

Loading

rozele commented Sep 22, 2022 •

edited

Loading

asklar Sep 22, 2022 •

edited

Loading

rozele Sep 22, 2022 •

edited

Loading