Move all metro logic and commands into a metro package #1447

afoxman · 2021-07-15T00:27:26Z

Summary:

I am working on a project where I need to do Metro bundling using the Metro APIs. The CLI has really useful Metro config and Metro bundling logic that I want to use, but it's tied up and not accessible.

This PR move the Metro configuration and bundling logic into separate packages, making them available for my project and to other external users.

Test Plan:

Code is moving, with minimal changes, including tests. Verify that the build passes and that all tests pass.

Manually verify that bundling works on a sample app. Diffed bundles using existing release of CLI vs this PR. Bundle size and md5 hashes were identical.

…making them available to external users.

grabbou · 2021-07-15T14:26:39Z

I love this! It's something I wanted to do for a really really long time!

Can you do one more thing, rename metro-config to just metro and move start as well as bundle commands into that package? This will be a good first step to eventually provide an abstraction for other bundlers to plug into CLI instead of Metro.

The idea is that one should be able too completely replace Metro with e.g. Webpack or other bundler by removing @react-native-community/bundler-metro package and using a different one instead.

CC: @tido64

packages/cli/src/tools/config/resolveNodeModuleDir.ts

afoxman · 2021-07-16T07:18:36Z

I love this! It's something I wanted to do for a really really long time!

Can you do one more thing, rename metro-config to just metro and move start as well as bundle commands into that package? This will be a good first step to eventually provide an abstraction for other bundlers to plug into CLI instead of Metro.

The idea is that one should be able too completely replace Metro with e.g. Webpack or other bundler by removing @react-native-community/bundler-metro package and using a different one instead.

CC: @tido64

Happy to help! This turned into a big PR, so you have a lot of reading to do :). The metro package now has all metro commands (start, bundle, and ramBundle) as well as the metro config/asset code.

I had to move a few "tools" from cli to cli-tools, so I could use them in metro. That made things a bit complicated and added to the PR.

grabbou · 2021-07-16T13:12:04Z

No worries, that's something I've been waiting for quite some time to see, so I am more than happy to review it!

grabbou · 2021-07-16T13:13:23Z

In the future release of React Native, I would like to discuss an opportunity of moving Metro dependency to a user project, so that they can define bundler and complementary CLI plugin for it.

afoxman · 2021-07-16T21:31:39Z

In the future release of React Native, I would like to discuss an opportunity of moving Metro dependency to a user project, so that they can define bundler and complementary CLI plugin for it.

That sounds good. We should sync up on what you have in mind, and how I can help. Sent you a friend request on Discord to work out the details in DMs.

afoxman · 2021-09-03T22:06:26Z

@thymikee @grabbou Hi folks - checking in to see when you might merge this, or if you'd like any further changes?

thymikee · 2021-09-13T11:15:11Z

I'm sure @grabbou will prioritize this the next time he works on this lib

grabbou · 2021-09-13T13:03:45Z

Yes, I am working on this this week. Will be reviewed on Wednesday.

grabbou · 2021-09-16T17:08:54Z

Update: I am still in progress with this one, expected update tomorrow

grabbou

Conceptually, everything looks fine. I have left some comments, but I believe this is going to be the final round.

TL;DR:

Let's not export "all" (via export * from 'foo.ts') and avoid creating index.ts in folders
Prefer export default instead of single named exports per file (sticks consistent with the rest of the codebase, happy to revisit later in a separate PR)
Export explicitly the public interface via src/index.ts (instead of *)
Do not change current exported/private functions beyond the loadMetroConfig.ts that is subject to this PR (unless there's a need for an additional function - then, it's welcome)

I am happy to hop on a Discord/Hangouts call to discuss this in a bit more details if necessary. Thank you for working on that!

packages/metro/src/bundle/buildBundle.ts

packages/metro/src/config/index.ts

packages/metro/src/index.ts

packages/metro/src/start/index.ts

packages/cli/package.json

afoxman · 2021-09-22T18:44:07Z

Conceptually, everything looks fine. I have left some comments, but I believe this is going to be the final round.

TL;DR:

Let's not export "all" (via export * from 'foo.ts') and avoid creating index.ts in folders

Prefer export default instead of single named exports per file (sticks consistent with the rest of the codebase, happy to revisit later in a separate PR)

Export explicitly the public interface via src/index.ts (instead of *)

Do not change current exported/private functions beyond the loadMetroConfig.ts that is subject to this PR (unless there's a need for an additional function - then, it's welcome)

I am happy to hop on a Discord/Hangouts call to discuss this in a bit more details if necessary. Thank you for working on that!

I think I've addressed all these points in my latest push. Use default exports. And all public API exports are now explicit, and only what @react-native-community/cli needs to consume (the command objects) and what I need to consume in @rnx-kit/* packages.

afoxman · 2021-09-22T19:27:00Z

Looks like an e2e test failed.

I can't log into circleci to find out details. It wants too much access to my Microsoft github account, so I can't sign in.

When I run 'yarn test' and 'yarn test:ci:e2e' (the command circleci runs), I get success. If I broke something, I'll need help diagnosing.

In the meantime, I tried merging the upstream master, to kick off another build to see if it fails with the latest commits.

grabbou · 2021-09-27T09:33:51Z

packages/cli/src/commands/index.ts

+  startCommand as start,
+  bundleCommand as bundle,
+  ramBundleCommand as ramBundle,
+} from '@react-native-community/cli-plugin-metro';


In the future, we can remove this when this plugin becomes direct React Native dependency.

Here's a snippet:
https://github.com/facebook/react-native/blob/main/react-native.config.js#L16

I believe it should simply say metro.commands, but this is to be decided on the planning meeting.

grabbou · 2021-09-27T09:49:29Z

I updated your PR with some further naming changes. The primary change was to group commands into commands folder (stays uniform with android and iOS plugins) and export them as an array. They're not supposed to be consumed as a public API, but used internally, and array is what the CLI infrastructure prefers.

Please let me know if you're happy with how it looks and whether the public API satisfies your use case.

…n a bit better

afoxman · 2021-09-27T23:42:12Z

packages/cli-plugin-metro/src/index.ts

-export {
-  bundleCommand,
-  ramBundleCommand,
-  buildBundleWithConfig,


I need buildBundleWithConfig to be publicly exported.

Added (along with CommandLineArgs type) in my latest push

afoxman · 2021-09-27T23:59:34Z

I updated your PR with some further naming changes. The primary change was to group commands into commands folder (stays uniform with android and iOS plugins) and export them as an array. They're not supposed to be consumed as a public API, but used internally, and array is what the CLI infrastructure prefers.

Please let me know if you're happy with how it looks and whether the public API satisfies your use case.

Everything looks ok to me. One function export and a type were removed during the shuffle, so I added them back in. I need to be able to call buildBundleWithConfig(...) and so I need it as well as its input type CommandLineArgs.

grabbou · 2021-10-04T09:06:43Z

LGTM.

grabbou · 2021-10-04T09:07:29Z

The CI failure looks unrelated. I will move forward and merge this PR.

Move metro configuration and bundling logic into their own packages, …

02e96a6

…making them available to external users.

afoxman requested review from Esemesek, grabbou and thymikee as code owners July 15, 2021 00:27

afoxman mentioned this pull request Jul 15, 2021

Use @react-native-communty/cli-plugin-metro project to bundling, and remove custom code in CLI and related packages microsoft/rnx-kit#328

Closed

afoxman added 6 commits July 15, 2021 10:13

Rename metro-config package to metro

76f019f

Move cli-bundle-api package and bundle command (cli package) into metro.

e55648c

Move resolveNodeModuleDir to cli-tools package

3fa4c4d

Move releaseChecker into cli-tools

4db4e31

Move hookStdout to cli-tools

1f359ef

Move start command into metro package

e1da75c

afoxman changed the title ~~Move bundle and metro config logic into separate packages~~ Move all metro logic and commands into a metro package Jul 16, 2021

Merge remote-tracking branch 'origin/master' into cli-bundle-api

7d46022

afoxman commented Jul 16, 2021

View reviewed changes

packages/cli/src/tools/config/resolveNodeModuleDir.ts Show resolved Hide resolved

grabbou requested changes Sep 20, 2021

View reviewed changes

afoxman added 6 commits September 20, 2021 14:32

Merge remote-tracking branch 'origin/master' into cli-bundle-api

d47dab3

Rename 'metro' package to 'cli-plugin-metro'.

75e00a2

Update dependency

11786fd

More renaming

7fa0133

Revert buildBundle to default export

d6462a3

Make exports explicit

9f88c58

afoxman added 2 commits September 21, 2021 15:11

Revert exports to default

c053379

PR feedback

83a290f

Merge branch 'react-native-community:master' into cli-bundle-api

18338dc

grabbou reviewed Sep 27, 2021

View reviewed changes

refactor(structure): moved some files around to reflect the conventio…

5ec6ae8

…n a bit better

afoxman commented Sep 27, 2021

View reviewed changes

Fix type bug, export missing funciton/type.

27f74ab

grabbou merged commit 0993f62 into react-native-community:master Oct 4, 2021

tido64 mentioned this pull request Aug 8, 2023

Migrate cli-plugin-metro into repo facebook/react-native#38795

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move all metro logic and commands into a metro package #1447

Move all metro logic and commands into a metro package #1447

afoxman commented Jul 15, 2021

grabbou commented Jul 15, 2021 •

edited

Loading

afoxman commented Jul 16, 2021

grabbou commented Jul 16, 2021

grabbou commented Jul 16, 2021

afoxman commented Jul 16, 2021

afoxman commented Sep 3, 2021

thymikee commented Sep 13, 2021

grabbou commented Sep 13, 2021

grabbou commented Sep 16, 2021

grabbou left a comment

afoxman commented Sep 22, 2021 •

edited

Loading

afoxman commented Sep 22, 2021 •

edited

Loading

grabbou Sep 27, 2021

grabbou commented Sep 27, 2021

afoxman Sep 27, 2021

afoxman Sep 27, 2021

afoxman commented Sep 27, 2021

grabbou commented Oct 4, 2021

grabbou commented Oct 4, 2021

Move all metro logic and commands into a metro package #1447

Move all metro logic and commands into a metro package #1447

Conversation

afoxman commented Jul 15, 2021

Summary:

Test Plan:

grabbou commented Jul 15, 2021 • edited Loading

afoxman commented Jul 16, 2021

grabbou commented Jul 16, 2021

grabbou commented Jul 16, 2021

afoxman commented Jul 16, 2021

afoxman commented Sep 3, 2021

thymikee commented Sep 13, 2021

grabbou commented Sep 13, 2021

grabbou commented Sep 16, 2021

grabbou left a comment

Choose a reason for hiding this comment

afoxman commented Sep 22, 2021 • edited Loading

afoxman commented Sep 22, 2021 • edited Loading

grabbou Sep 27, 2021

Choose a reason for hiding this comment

grabbou commented Sep 27, 2021

afoxman Sep 27, 2021

Choose a reason for hiding this comment

afoxman Sep 27, 2021

Choose a reason for hiding this comment

afoxman commented Sep 27, 2021

grabbou commented Oct 4, 2021

grabbou commented Oct 4, 2021

grabbou commented Jul 15, 2021 •

edited

Loading

afoxman commented Sep 22, 2021 •

edited

Loading

afoxman commented Sep 22, 2021 •

edited

Loading