Skip to content

Conversation

tido64
Copy link
Collaborator

@tido64 tido64 commented Sep 14, 2023

Summary:

Transpile assets-registry so it can be consumed outside of a React Native bundle:

  • Copied root .js files into src
  • Declared exports in package.json
  • To keep backwards compatibility, the root .js files re-export the corresponding files in the src folder

This is a follow-up to #38795 (comment). Let me know if this looks feasible to you.

Changelog:

[INTERNAL] [CHANGED] - Transpile assets-registry so it can be consumed outside of a React Native bundle

Test Plan:

n/a

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Microsoft Partner: Microsoft Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Sep 14, 2023
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,970,014 +19
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 9,562,448 +50
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: b29b393
Branch: main

@cortinico
Copy link
Contributor

cc @huntie

Copy link
Member

@huntie huntie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for migrating this package into the shared build setup 😄.

nit: Changelog in PR summary can be reduced to Changelog: [Internal].

Comment on lines +20 to +23
"dist",
"path-support.js",
"registry.js",
"src"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"dist",
"path-support.js",
"registry.js",
"src"
"dist",

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This suggestion still applies — all other files do not need to be packaged, but are instead present at dev-time.

getAndroidResourceIdentifier,
getBasePath,
};
module.exports = require('./src/path-support');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To enable running from source, let's add babel-register wrapper files for these two entry points. And yeah, actually that means keeping these files around (for Flow's type checker, which sadly doesn't follow "exports").

You can find an example of this pattern in packages/dev-middleware/: https://github.com/facebook/react-native/blob/main/packages/dev-middleware/src/index.js.

And the file structure you'll need:

assets
├── path-support.js.flow # (Flow only) Re-export all types and values
├── registry.js.flow
└── src
    ├── path-support.js # Entry point - babel-register
    ├── path-support.flow.js # Implementation
    ├── registry.js
    └── registry.flow.js

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I didn't quite understand what those files were. Makes sense now!

Copy link
Collaborator Author

@tido64 tido64 Sep 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So… it looks like flow-check is unable to find .js.flow files. I had to rename them back to .js and fix a bunch of Dependencies of a @flow strict module must also be @flow strict! errors. The check seems to pass locally now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this pattern works. babel-register cannot be found when the package is consumed outside the repository. Do you have any suggestions for how to address this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

babel-register cannot be foundwhen the package is consumed outside the repository.

Do you mean you have a setup where another project is consuming a local checkout of the react-native repository from source (e.g. via yarn link)?

This should work. Happy to debug via chat.

@tido64 tido64 force-pushed the tido/build-assets-registry branch from ade331e to 16a72c5 Compare September 14, 2023 18:36
@github-actions
Copy link

Warnings
⚠️ One hour and a half have passed and the E2E jobs haven't finished yet.

Generated by 🚫 dangerJS against 9ba56eb

Comment on lines +7 to +8
* @format strict
* @flow
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this line was accidentally rearranged first!

Also, feel free to use strict-local everywhere, it's slightly more forgiving.

Suggested change
* @format strict
* @flow
* @flow strict-local
* @format

getAndroidResourceIdentifier,
getBasePath,
};
module.exports = require('./src/path-support');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

babel-register cannot be foundwhen the package is consumed outside the repository.

Do you mean you have a setup where another project is consuming a local checkout of the react-native repository from source (e.g. via yarn link)?

This should work. Happy to debug via chat.

Comment on lines 11 to +13
'use strict';

export type PackagerAsset = {
+__packager_asset: boolean,
+fileSystemLocation: string,
+httpServerLocation: string,
+width: ?number,
+height: ?number,
+scales: Array<number>,
+hash: string,
+name: string,
+type: string,
...
};

const assets: Array<PackagerAsset> = [];

function registerAsset(asset: PackagerAsset): number {
// `push` returns new array length, so the first asset will
// get id 1 (not 0) to make the value truthy
return assets.push(asset);
}

function getAssetByID(assetId: number): PackagerAsset {
return assets[assetId - 1];
}

module.exports = {registerAsset, getAssetByID};
module.exports = require('./src/registry.flow');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'use strict';
export type PackagerAsset = {
+__packager_asset: boolean,
+fileSystemLocation: string,
+httpServerLocation: string,
+width: ?number,
+height: ?number,
+scales: Array<number>,
+hash: string,
+name: string,
+type: string,
...
};
const assets: Array<PackagerAsset> = [];
function registerAsset(asset: PackagerAsset): number {
// `push` returns new array length, so the first asset will
// get id 1 (not 0) to make the value truthy
return assets.push(asset);
}
function getAssetByID(assetId: number): PackagerAsset {
return assets[assetId - 1];
}
module.exports = {registerAsset, getAssetByID};
module.exports = require('./src/registry.flow');
export * from './src/registry.flow';

Comment on lines +20 to +23
"dist",
"path-support.js",
"registry.js",
"src"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This suggestion still applies — all other files do not need to be packaged, but are instead present at dev-time.

getAndroidResourceIdentifier,
getBasePath,
};
module.exports = require('./src/path-support');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
module.exports = require('./src/path-support');
export * from './src/path-support';

// Make Metro able to resolve required external dependencies
watchFolders: [
path.resolve(__dirname, '../../node_modules'),
path.resolve(__dirname, '../../scripts/build'), // babel-register.js
Copy link
Member

@huntie huntie Sep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wait, @react-native/asset-registry is not a Node package — it's consumed by runtime code in packages/react-native/Libraries/!

This probably means we don't need the babel-register setup at all (just the root .js.flow files redirecting to src/) — since all modules loaded by React Native go through @react-native/babel-preset. This setup is just for run-from-source behaviour within the React Native monorepo.

Back to the PR summary:

Transpile assets-registry so it can be consumed outside of a React Native bundle:

This is technically new ground. I think we want to define a new build target of 'react-native'/'react-native-and-external' in scripts/build/config.js, configuring this to build files using @react-native/babel-preset as its Babel config. Will reach out via DM.

@zhairui
Copy link

zhairui commented Jan 25, 2024

cc @huntie

I meet an error , pls help . thanks ! here is the error stack:

[TypeError: _$$_REQUIRE(_dependencyMap[7], "(...)ssets-registry/path-support.js") is not a function (it is Object)] {"componentStack": "
in BaseImage (created by ImageBackground)
in RCTView (created by View)
in View (created by ImageBackground)
....

@react-native-bot
Copy link
Collaborator

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@react-native-bot react-native-bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Jul 24, 2024
@react-native-bot
Copy link
Collaborator

This PR was closed because it has been stalled for 7 days with no activity.

@tido64 tido64 deleted the tido/build-assets-registry branch February 11, 2025 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Microsoft Partner: Microsoft Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. Stale There has been a lack of activity on this issue and it may be closed soon.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants