Expose unstable_InspectorProxy and unstable_Device from dev-middleware

[gabrieldonadel]

Summary:

Recently, both metro-inspector-proxy(#39045) and @react-native-community/cli-plugin-metro(#38795) were moved to this repo and in the process of moving these packages, the exports field inside package.json was added, only exporting the index.js file.

The problem is that Expo CLI (and possibly other community packages) rely on functions and classes that are not exported in the index.js file, e.g. Importing the InspectorProxy class from @react-native/dev-middleware/dist/inspector-proxy/InspectorProxy. Normally this wouldn't be a problem and we would just import from dist/ but due to the exports field, attempting to import from any other file not specified on this field will result in a ERR_PACKAGE_PATH_NOT_EXPORTED error.

As a short-term fix, we should create unstable_-prefixed exports of individual features Expo currently depends on.

Changelog:

[INTERNAL] [CHANGED] - Expose unstable_InspectorProxy and unstable_Device from @react-native/dev-middleware

Test Plan:

N / A

[motiz88]

Hey @gabrieldonadel, let's talk about alternatives to this PR. Generally speaking, strengthening the encapsulation of these packages was an intentional change to allow us to iterate faster, and removing exports would undo that.

Expo CLI should have a plan for moving off its dependencies on the internals of these packages, unless we make them intentionally public. Short term, we can be pragmatic here and create unstable_-prefixed exports of individual features Expo currently depends on, but you should expect them to be removed or otherwise break in a future release.

You mentioned InspectorProxy specifically. Note that we're actively working on this class and considering it an implementation detail of @react-native/dev-middleware. It's my understanding that in the long term, Expo CLI will use @react-native/dev-middleware directly and not depend on customising InspectorProxy. (cc @byCedric, @huntie)

Can you elaborate on other specific use cases for deep imports from these packages, and the plan for moving away from them?

[EvanBacon]

For all dev tools packages (minus CLIs), I'd prefer if we could reach the internals as the iteration cycles would cause us potentially weeks/months of delay to get changes out to users, which would cause us to either delay react native upgrades or need to fork more code away from the community packages.

Since debugging is fairly stable in Expo CLI at the moment and we're currently in the middle of the react-native upgrade process, we could update this PR to expose all of the internal APIs we're using, either through the main export or package exports.

[byCedric]

I can speak for the use case of customizing the InspectorProxy and Device classes. We currently extend these two classes to add functionality, mainly in two categories:

• Add new functionality, like the Network inspector.
• Add workarounds/fixes to avoid hard crashes with Hermes.

For the network inspector, we can only move off our extended proxy when the new native Hermes CDP backend is ready and we re-implemented the network inspector in the new system.

For the workaround / fixes, we need Hermes to stop crashing apps on any CDP message. Even when the CDP message content is malformed. E.g. Runtime.callFunctionOn could cause a crash when injecting a JS script using features not available in Hermes (see expo/vscode-expo#231 and expo/expo#25270 for the latest example, and all other Vscode... handlers to make vscode CDP messages not crash the app)

I think the best path forward on our end would be to keep extending these classes until the newer Hermes CDP backend is in place, and we migrated or upstreamed the network inspector.

For now, I think having access to the Device and InspectorProxy class, and it's initialization is the in-between step. If we could use createDevMiddleware with our own InspectorProxy class, we can switch to this in-between step and start consuming the @react-native/dev-middleware package. I believe this requirement around the InspectorProxy class was also what @huntie and I have discussed earlier, but correct me if I'm wrong 😄

[gabrieldonadel]

Regarding @react-native/community-cli-plugin we will no longer need to change it as we were only using the saveAssets function and Evan just opened a PR updating the CLI to no longer use it expo/expo#25278

[github-actions]

	Warnings
⚠️	packages/dev-middleware/src/createDevMiddleware.js#L12 - packages/dev-middleware/src/createDevMiddleware.js line 12 – Requires should be sorted alphabetically (lint/sort-imports)

Generated by 🚫 dangerJS against 3c25924

[motiz88]

@byCedric's updates to this PR (exposing unstable_Device, unstable_InspectorProxy as top-level exports, adding an unstable config option to replace the InspectorProxy implementation) make perfect sense to me for React Native 0.73. I've asked @blakef to help with landing this and getting it picked into the next 0.73 RC.

For 0.74 onwards, can we be proactive about moving away from these unstable mechanisms, and addressing the underlying problems in a sustainable way? Specifically:

we need Hermes to stop crashing apps on any CDP message

If this is a bug in Hermes's CDP handler, let's just fix it (cc @dannysu) and put unit tests around it - this doesn't have to be blocked on shipping the entire new CDP stack, and doesn't warrant keeping around a monkey-patched version of InspectorProxy.

For the network inspector, we can only move off our extended proxy when the new native Hermes CDP backend is ready and we re-implemented the network inspector in the new system.

Let's derisk this. Adding a native network inspector inside RN is still an important goal for us, but it's likely the new CDP stack will ship without it at first. In that world, continuing to use a monkey-patched InspectorProxy is a reliability risk. Can we minimally port Expo's existing network inspector to a supported, tested API in InspectorProxy and stop relying on monkey-patching? (Note that InspectorProxy has an actual test suite now.) Even if that API is itself unstable_ and we still have the goal of migrating to a fully native solution.

[facebook-github-bot]

@blakef has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[github-actions]

This pull request was successfully merged by @gabrieldonadel in 1a61afd.

^{When will my fix make it into a release? | Upcoming Releases}

[gabrieldonadel]

Summary:

Recently, both metro-inspector-proxy(#39045) and @react-native-community/cli-plugin-metro(#38795) were moved to this repo and in the process of moving these packages, the exports field inside package.json was added, only exporting the index.js file.

The problem is that Expo CLI (and possibly other community packages) rely on functions and classes that are not exported in the index.js file, e.g. Importing the InspectorProxy class from @react-native/dev-middleware/dist/inspector-proxy/InspectorProxy. Normally this wouldn't be a problem and we would just import from dist/ but due to the exports field, attempting to import from any other file not specified on this field will result in a ERR_PACKAGE_PATH_NOT_EXPORTED error.

As a short-term fix, we should create unstable_-prefixed exports of individual features Expo currently depends on.

Changelog:

[INTERNAL] [CHANGED] - Expose unstable_InspectorProxy and unstable_Device from @react-native/dev-middleware

Test Plan:

N / A

[motiz88]

Hey @gabrieldonadel, let's talk about alternatives to this PR. Generally speaking, strengthening the encapsulation of these packages was an intentional change to allow us to iterate faster, and removing exports would undo that.

Expo CLI should have a plan for moving off its dependencies on the internals of these packages, unless we make them intentionally public. Short term, we can be pragmatic here and create unstable_-prefixed exports of individual features Expo currently depends on, but you should expect them to be removed or otherwise break in a future release.

You mentioned InspectorProxy specifically. Note that we're actively working on this class and considering it an implementation detail of @react-native/dev-middleware. It's my understanding that in the long term, Expo CLI will use @react-native/dev-middleware directly and not depend on customising InspectorProxy. (cc @byCedric, @huntie)

Can you elaborate on other specific use cases for deep imports from these packages, and the plan for moving away from them?

[EvanBacon]

For all dev tools packages (minus CLIs), I'd prefer if we could reach the internals as the iteration cycles would cause us potentially weeks/months of delay to get changes out to users, which would cause us to either delay react native upgrades or need to fork more code away from the community packages.

Since debugging is fairly stable in Expo CLI at the moment and we're currently in the middle of the react-native upgrade process, we could update this PR to expose all of the internal APIs we're using, either through the main export or package exports.

[byCedric]

I can speak for the use case of customizing the InspectorProxy and Device classes. We currently extend these two classes to add functionality, mainly in two categories:

• Add new functionality, like the Network inspector.
• Add workarounds/fixes to avoid hard crashes with Hermes.

For the network inspector, we can only move off our extended proxy when the new native Hermes CDP backend is ready and we re-implemented the network inspector in the new system.

For the workaround / fixes, we need Hermes to stop crashing apps on any CDP message. Even when the CDP message content is malformed. E.g. Runtime.callFunctionOn could cause a crash when injecting a JS script using features not available in Hermes (see expo/vscode-expo#231 and expo/expo#25270 for the latest example, and all other Vscode... handlers to make vscode CDP messages not crash the app)

I think the best path forward on our end would be to keep extending these classes until the newer Hermes CDP backend is in place, and we migrated or upstreamed the network inspector.

For now, I think having access to the Device and InspectorProxy class, and it's initialization is the in-between step. If we could use createDevMiddleware with our own InspectorProxy class, we can switch to this in-between step and start consuming the @react-native/dev-middleware package. I believe this requirement around the InspectorProxy class was also what @huntie and I have discussed earlier, but correct me if I'm wrong 😄

[gabrieldonadel]

Regarding @react-native/community-cli-plugin we will no longer need to change it as we were only using the saveAssets function and Evan just opened a PR updating the CLI to no longer use it expo/expo#25278

[github-actions]

	Warnings
⚠️	packages/dev-middleware/src/createDevMiddleware.js#L12 - packages/dev-middleware/src/createDevMiddleware.js line 12 – Requires should be sorted alphabetically (lint/sort-imports)

Generated by 🚫 dangerJS against 3c25924

[motiz88]

@byCedric's updates to this PR (exposing unstable_Device, unstable_InspectorProxy as top-level exports, adding an unstable config option to replace the InspectorProxy implementation) make perfect sense to me for React Native 0.73. I've asked @blakef to help with landing this and getting it picked into the next 0.73 RC.

For 0.74 onwards, can we be proactive about moving away from these unstable mechanisms, and addressing the underlying problems in a sustainable way? Specifically:

we need Hermes to stop crashing apps on any CDP message

If this is a bug in Hermes's CDP handler, let's just fix it (cc @dannysu) and put unit tests around it - this doesn't have to be blocked on shipping the entire new CDP stack, and doesn't warrant keeping around a monkey-patched version of InspectorProxy.

For the network inspector, we can only move off our extended proxy when the new native Hermes CDP backend is ready and we re-implemented the network inspector in the new system.

Let's derisk this. Adding a native network inspector inside RN is still an important goal for us, but it's likely the new CDP stack will ship without it at first. In that world, continuing to use a monkey-patched InspectorProxy is a reliability risk. Can we minimally port Expo's existing network inspector to a supported, tested API in InspectorProxy and stop relying on monkey-patching? (Note that InspectorProxy has an actual test suite now.) Even if that API is itself unstable_ and we still have the goal of migrating to a fully native solution.

[facebook-github-bot]

@blakef has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[github-actions]

This pull request was successfully merged by @gabrieldonadel in 1a61afd.

^{When will my fix make it into a release? | Upcoming Releases}

[gabrieldonadel]

Summary:

Recently, both metro-inspector-proxy(#39045) and @react-native-community/cli-plugin-metro(#38795) were moved to this repo and in the process of moving these packages, the exports field inside package.json was added, only exporting the index.js file.

The problem is that Expo CLI (and possibly other community packages) rely on functions and classes that are not exported in the index.js file, e.g. Importing the InspectorProxy class from @react-native/dev-middleware/dist/inspector-proxy/InspectorProxy. Normally this wouldn't be a problem and we would just import from dist/ but due to the exports field, attempting to import from any other file not specified on this field will result in a ERR_PACKAGE_PATH_NOT_EXPORTED error.

As a short-term fix, we should create unstable_-prefixed exports of individual features Expo currently depends on.

Changelog:

[INTERNAL] [CHANGED] - Expose unstable_InspectorProxy and unstable_Device from @react-native/dev-middleware

Test Plan:

N / A

[motiz88]

Hey @gabrieldonadel, let's talk about alternatives to this PR. Generally speaking, strengthening the encapsulation of these packages was an intentional change to allow us to iterate faster, and removing exports would undo that.

Expo CLI should have a plan for moving off its dependencies on the internals of these packages, unless we make them intentionally public. Short term, we can be pragmatic here and create unstable_-prefixed exports of individual features Expo currently depends on, but you should expect them to be removed or otherwise break in a future release.

You mentioned InspectorProxy specifically. Note that we're actively working on this class and considering it an implementation detail of @react-native/dev-middleware. It's my understanding that in the long term, Expo CLI will use @react-native/dev-middleware directly and not depend on customising InspectorProxy. (cc @byCedric, @huntie)

Can you elaborate on other specific use cases for deep imports from these packages, and the plan for moving away from them?

[EvanBacon]

For all dev tools packages (minus CLIs), I'd prefer if we could reach the internals as the iteration cycles would cause us potentially weeks/months of delay to get changes out to users, which would cause us to either delay react native upgrades or need to fork more code away from the community packages.

Since debugging is fairly stable in Expo CLI at the moment and we're currently in the middle of the react-native upgrade process, we could update this PR to expose all of the internal APIs we're using, either through the main export or package exports.

[byCedric]

I can speak for the use case of customizing the InspectorProxy and Device classes. We currently extend these two classes to add functionality, mainly in two categories:

• Add new functionality, like the Network inspector.
• Add workarounds/fixes to avoid hard crashes with Hermes.

For the network inspector, we can only move off our extended proxy when the new native Hermes CDP backend is ready and we re-implemented the network inspector in the new system.

For the workaround / fixes, we need Hermes to stop crashing apps on any CDP message. Even when the CDP message content is malformed. E.g. Runtime.callFunctionOn could cause a crash when injecting a JS script using features not available in Hermes (see expo/vscode-expo#231 and expo/expo#25270 for the latest example, and all other Vscode... handlers to make vscode CDP messages not crash the app)

I think the best path forward on our end would be to keep extending these classes until the newer Hermes CDP backend is in place, and we migrated or upstreamed the network inspector.

For now, I think having access to the Device and InspectorProxy class, and it's initialization is the in-between step. If we could use createDevMiddleware with our own InspectorProxy class, we can switch to this in-between step and start consuming the @react-native/dev-middleware package. I believe this requirement around the InspectorProxy class was also what @huntie and I have discussed earlier, but correct me if I'm wrong 😄

[gabrieldonadel]

Regarding @react-native/community-cli-plugin we will no longer need to change it as we were only using the saveAssets function and Evan just opened a PR updating the CLI to no longer use it expo/expo#25278

[github-actions]

	Warnings
⚠️	packages/dev-middleware/src/createDevMiddleware.js#L12 - packages/dev-middleware/src/createDevMiddleware.js line 12 – Requires should be sorted alphabetically (lint/sort-imports)

Generated by 🚫 dangerJS against 3c25924

[motiz88]

@byCedric's updates to this PR (exposing unstable_Device, unstable_InspectorProxy as top-level exports, adding an unstable config option to replace the InspectorProxy implementation) make perfect sense to me for React Native 0.73. I've asked @blakef to help with landing this and getting it picked into the next 0.73 RC.

For 0.74 onwards, can we be proactive about moving away from these unstable mechanisms, and addressing the underlying problems in a sustainable way? Specifically:

we need Hermes to stop crashing apps on any CDP message

If this is a bug in Hermes's CDP handler, let's just fix it (cc @dannysu) and put unit tests around it - this doesn't have to be blocked on shipping the entire new CDP stack, and doesn't warrant keeping around a monkey-patched version of InspectorProxy.

For the network inspector, we can only move off our extended proxy when the new native Hermes CDP backend is ready and we re-implemented the network inspector in the new system.

Let's derisk this. Adding a native network inspector inside RN is still an important goal for us, but it's likely the new CDP stack will ship without it at first. In that world, continuing to use a monkey-patched InspectorProxy is a reliability risk. Can we minimally port Expo's existing network inspector to a supported, tested API in InspectorProxy and stop relying on monkey-patching? (Note that InspectorProxy has an actual test suite now.) Even if that API is itself unstable_ and we still have the goal of migrating to a fully native solution.

[facebook-github-bot]

@blakef has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[github-actions]

This pull request was successfully merged by @gabrieldonadel in 1a61afd.

^{When will my fix make it into a release? | Upcoming Releases}

[gabrieldonadel]

Summary:

Recently, both metro-inspector-proxy(#39045) and @react-native-community/cli-plugin-metro(#38795) were moved to this repo and in the process of moving these packages, the exports field inside package.json was added, only exporting the index.js file.

The problem is that Expo CLI (and possibly other community packages) rely on functions and classes that are not exported in the index.js file, e.g. Importing the InspectorProxy class from @react-native/dev-middleware/dist/inspector-proxy/InspectorProxy. Normally this wouldn't be a problem and we would just import from dist/ but due to the exports field, attempting to import from any other file not specified on this field will result in a ERR_PACKAGE_PATH_NOT_EXPORTED error.

As a short-term fix, we should create unstable_-prefixed exports of individual features Expo currently depends on.

Changelog:

[INTERNAL] [CHANGED] - Expose unstable_InspectorProxy and unstable_Device from @react-native/dev-middleware

Test Plan:

N / A

[motiz88]

Hey @gabrieldonadel, let's talk about alternatives to this PR. Generally speaking, strengthening the encapsulation of these packages was an intentional change to allow us to iterate faster, and removing exports would undo that.

Expo CLI should have a plan for moving off its dependencies on the internals of these packages, unless we make them intentionally public. Short term, we can be pragmatic here and create unstable_-prefixed exports of individual features Expo currently depends on, but you should expect them to be removed or otherwise break in a future release.

You mentioned InspectorProxy specifically. Note that we're actively working on this class and considering it an implementation detail of @react-native/dev-middleware. It's my understanding that in the long term, Expo CLI will use @react-native/dev-middleware directly and not depend on customising InspectorProxy. (cc @byCedric, @huntie)

Can you elaborate on other specific use cases for deep imports from these packages, and the plan for moving away from them?

[EvanBacon]

For all dev tools packages (minus CLIs), I'd prefer if we could reach the internals as the iteration cycles would cause us potentially weeks/months of delay to get changes out to users, which would cause us to either delay react native upgrades or need to fork more code away from the community packages.

Since debugging is fairly stable in Expo CLI at the moment and we're currently in the middle of the react-native upgrade process, we could update this PR to expose all of the internal APIs we're using, either through the main export or package exports.

[byCedric]

I can speak for the use case of customizing the InspectorProxy and Device classes. We currently extend these two classes to add functionality, mainly in two categories:

• Add new functionality, like the Network inspector.
• Add workarounds/fixes to avoid hard crashes with Hermes.

For the network inspector, we can only move off our extended proxy when the new native Hermes CDP backend is ready and we re-implemented the network inspector in the new system.

For the workaround / fixes, we need Hermes to stop crashing apps on any CDP message. Even when the CDP message content is malformed. E.g. Runtime.callFunctionOn could cause a crash when injecting a JS script using features not available in Hermes (see expo/vscode-expo#231 and expo/expo#25270 for the latest example, and all other Vscode... handlers to make vscode CDP messages not crash the app)

I think the best path forward on our end would be to keep extending these classes until the newer Hermes CDP backend is in place, and we migrated or upstreamed the network inspector.

For now, I think having access to the Device and InspectorProxy class, and it's initialization is the in-between step. If we could use createDevMiddleware with our own InspectorProxy class, we can switch to this in-between step and start consuming the @react-native/dev-middleware package. I believe this requirement around the InspectorProxy class was also what @huntie and I have discussed earlier, but correct me if I'm wrong 😄

[gabrieldonadel]

Regarding @react-native/community-cli-plugin we will no longer need to change it as we were only using the saveAssets function and Evan just opened a PR updating the CLI to no longer use it expo/expo#25278

[github-actions]

	Warnings
⚠️	packages/dev-middleware/src/createDevMiddleware.js#L12 - packages/dev-middleware/src/createDevMiddleware.js line 12 – Requires should be sorted alphabetically (lint/sort-imports)

Generated by 🚫 dangerJS against 3c25924

[motiz88]

@byCedric's updates to this PR (exposing unstable_Device, unstable_InspectorProxy as top-level exports, adding an unstable config option to replace the InspectorProxy implementation) make perfect sense to me for React Native 0.73. I've asked @blakef to help with landing this and getting it picked into the next 0.73 RC.

For 0.74 onwards, can we be proactive about moving away from these unstable mechanisms, and addressing the underlying problems in a sustainable way? Specifically:

we need Hermes to stop crashing apps on any CDP message

If this is a bug in Hermes's CDP handler, let's just fix it (cc @dannysu) and put unit tests around it - this doesn't have to be blocked on shipping the entire new CDP stack, and doesn't warrant keeping around a monkey-patched version of InspectorProxy.

For the network inspector, we can only move off our extended proxy when the new native Hermes CDP backend is ready and we re-implemented the network inspector in the new system.

Let's derisk this. Adding a native network inspector inside RN is still an important goal for us, but it's likely the new CDP stack will ship without it at first. In that world, continuing to use a monkey-patched InspectorProxy is a reliability risk. Can we minimally port Expo's existing network inspector to a supported, tested API in InspectorProxy and stop relying on monkey-patching? (Note that InspectorProxy has an actual test suite now.) Even if that API is itself unstable_ and we still have the goal of migrating to a fully native solution.

[facebook-github-bot]

@blakef has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[github-actions]

This pull request was successfully merged by @gabrieldonadel in 1a61afd.

^{When will my fix make it into a release? | Upcoming Releases}

[gabrieldonadel]

Summary:

Recently, both metro-inspector-proxy(#39045) and @react-native-community/cli-plugin-metro(#38795) were moved to this repo and in the process of moving these packages, the exports field inside package.json was added, only exporting the index.js file.

The problem is that Expo CLI (and possibly other community packages) rely on functions and classes that are not exported in the index.js file, e.g. Importing the InspectorProxy class from @react-native/dev-middleware/dist/inspector-proxy/InspectorProxy. Normally this wouldn't be a problem and we would just import from dist/ but due to the exports field, attempting to import from any other file not specified on this field will result in a ERR_PACKAGE_PATH_NOT_EXPORTED error.

As a short-term fix, we should create unstable_-prefixed exports of individual features Expo currently depends on.

Changelog:

[INTERNAL] [CHANGED] - Expose unstable_InspectorProxy and unstable_Device from @react-native/dev-middleware

Test Plan:

N / A

[motiz88]

Hey @gabrieldonadel, let's talk about alternatives to this PR. Generally speaking, strengthening the encapsulation of these packages was an intentional change to allow us to iterate faster, and removing exports would undo that.

Expo CLI should have a plan for moving off its dependencies on the internals of these packages, unless we make them intentionally public. Short term, we can be pragmatic here and create unstable_-prefixed exports of individual features Expo currently depends on, but you should expect them to be removed or otherwise break in a future release.

You mentioned InspectorProxy specifically. Note that we're actively working on this class and considering it an implementation detail of @react-native/dev-middleware. It's my understanding that in the long term, Expo CLI will use @react-native/dev-middleware directly and not depend on customising InspectorProxy. (cc @byCedric, @huntie)

Can you elaborate on other specific use cases for deep imports from these packages, and the plan for moving away from them?

[EvanBacon]

For all dev tools packages (minus CLIs), I'd prefer if we could reach the internals as the iteration cycles would cause us potentially weeks/months of delay to get changes out to users, which would cause us to either delay react native upgrades or need to fork more code away from the community packages.

Since debugging is fairly stable in Expo CLI at the moment and we're currently in the middle of the react-native upgrade process, we could update this PR to expose all of the internal APIs we're using, either through the main export or package exports.

[byCedric]

I can speak for the use case of customizing the InspectorProxy and Device classes. We currently extend these two classes to add functionality, mainly in two categories:

• Add new functionality, like the Network inspector.
• Add workarounds/fixes to avoid hard crashes with Hermes.

For the network inspector, we can only move off our extended proxy when the new native Hermes CDP backend is ready and we re-implemented the network inspector in the new system.

For the workaround / fixes, we need Hermes to stop crashing apps on any CDP message. Even when the CDP message content is malformed. E.g. Runtime.callFunctionOn could cause a crash when injecting a JS script using features not available in Hermes (see expo/vscode-expo#231 and expo/expo#25270 for the latest example, and all other Vscode... handlers to make vscode CDP messages not crash the app)

I think the best path forward on our end would be to keep extending these classes until the newer Hermes CDP backend is in place, and we migrated or upstreamed the network inspector.

For now, I think having access to the Device and InspectorProxy class, and it's initialization is the in-between step. If we could use createDevMiddleware with our own InspectorProxy class, we can switch to this in-between step and start consuming the @react-native/dev-middleware package. I believe this requirement around the InspectorProxy class was also what @huntie and I have discussed earlier, but correct me if I'm wrong 😄

[gabrieldonadel]

Regarding @react-native/community-cli-plugin we will no longer need to change it as we were only using the saveAssets function and Evan just opened a PR updating the CLI to no longer use it expo/expo#25278

[github-actions]

	Warnings
⚠️	packages/dev-middleware/src/createDevMiddleware.js#L12 - packages/dev-middleware/src/createDevMiddleware.js line 12 – Requires should be sorted alphabetically (lint/sort-imports)

Generated by 🚫 dangerJS against 3c25924

[motiz88]

@byCedric's updates to this PR (exposing unstable_Device, unstable_InspectorProxy as top-level exports, adding an unstable config option to replace the InspectorProxy implementation) make perfect sense to me for React Native 0.73. I've asked @blakef to help with landing this and getting it picked into the next 0.73 RC.

For 0.74 onwards, can we be proactive about moving away from these unstable mechanisms, and addressing the underlying problems in a sustainable way? Specifically:

we need Hermes to stop crashing apps on any CDP message

If this is a bug in Hermes's CDP handler, let's just fix it (cc @dannysu) and put unit tests around it - this doesn't have to be blocked on shipping the entire new CDP stack, and doesn't warrant keeping around a monkey-patched version of InspectorProxy.

For the network inspector, we can only move off our extended proxy when the new native Hermes CDP backend is ready and we re-implemented the network inspector in the new system.

Let's derisk this. Adding a native network inspector inside RN is still an important goal for us, but it's likely the new CDP stack will ship without it at first. In that world, continuing to use a monkey-patched InspectorProxy is a reliability risk. Can we minimally port Expo's existing network inspector to a supported, tested API in InspectorProxy and stop relying on monkey-patching? (Note that InspectorProxy has an actual test suite now.) Even if that API is itself unstable_ and we still have the goal of migrating to a fully native solution.

[facebook-github-bot]

@blakef has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[github-actions]

This pull request was successfully merged by @gabrieldonadel in 1a61afd.

^{When will my fix make it into a release? | Upcoming Releases}

[gabrieldonadel]

Summary:

Recently, both metro-inspector-proxy(#39045) and @react-native-community/cli-plugin-metro(#38795) were moved to this repo and in the process of moving these packages, the exports field inside package.json was added, only exporting the index.js file.

The problem is that Expo CLI (and possibly other community packages) rely on functions and classes that are not exported in the index.js file, e.g. Importing the InspectorProxy class from @react-native/dev-middleware/dist/inspector-proxy/InspectorProxy. Normally this wouldn't be a problem and we would just import from dist/ but due to the exports field, attempting to import from any other file not specified on this field will result in a ERR_PACKAGE_PATH_NOT_EXPORTED error.

As a short-term fix, we should create unstable_-prefixed exports of individual features Expo currently depends on.

Changelog:

[INTERNAL] [CHANGED] - Expose unstable_InspectorProxy and unstable_Device from @react-native/dev-middleware

Test Plan:

N / A

[motiz88]

Hey @gabrieldonadel, let's talk about alternatives to this PR. Generally speaking, strengthening the encapsulation of these packages was an intentional change to allow us to iterate faster, and removing exports would undo that.

Expo CLI should have a plan for moving off its dependencies on the internals of these packages, unless we make them intentionally public. Short term, we can be pragmatic here and create unstable_-prefixed exports of individual features Expo currently depends on, but you should expect them to be removed or otherwise break in a future release.

You mentioned InspectorProxy specifically. Note that we're actively working on this class and considering it an implementation detail of @react-native/dev-middleware. It's my understanding that in the long term, Expo CLI will use @react-native/dev-middleware directly and not depend on customising InspectorProxy. (cc @byCedric, @huntie)

Can you elaborate on other specific use cases for deep imports from these packages, and the plan for moving away from them?

[EvanBacon]

For all dev tools packages (minus CLIs), I'd prefer if we could reach the internals as the iteration cycles would cause us potentially weeks/months of delay to get changes out to users, which would cause us to either delay react native upgrades or need to fork more code away from the community packages.

Since debugging is fairly stable in Expo CLI at the moment and we're currently in the middle of the react-native upgrade process, we could update this PR to expose all of the internal APIs we're using, either through the main export or package exports.

[byCedric]

I can speak for the use case of customizing the InspectorProxy and Device classes. We currently extend these two classes to add functionality, mainly in two categories:

• Add new functionality, like the Network inspector.
• Add workarounds/fixes to avoid hard crashes with Hermes.

For the network inspector, we can only move off our extended proxy when the new native Hermes CDP backend is ready and we re-implemented the network inspector in the new system.

For the workaround / fixes, we need Hermes to stop crashing apps on any CDP message. Even when the CDP message content is malformed. E.g. Runtime.callFunctionOn could cause a crash when injecting a JS script using features not available in Hermes (see expo/vscode-expo#231 and expo/expo#25270 for the latest example, and all other Vscode... handlers to make vscode CDP messages not crash the app)

I think the best path forward on our end would be to keep extending these classes until the newer Hermes CDP backend is in place, and we migrated or upstreamed the network inspector.

For now, I think having access to the Device and InspectorProxy class, and it's initialization is the in-between step. If we could use createDevMiddleware with our own InspectorProxy class, we can switch to this in-between step and start consuming the @react-native/dev-middleware package. I believe this requirement around the InspectorProxy class was also what @huntie and I have discussed earlier, but correct me if I'm wrong 😄

[gabrieldonadel]

Regarding @react-native/community-cli-plugin we will no longer need to change it as we were only using the saveAssets function and Evan just opened a PR updating the CLI to no longer use it expo/expo#25278

[github-actions]

	Warnings
⚠️	packages/dev-middleware/src/createDevMiddleware.js#L12 - packages/dev-middleware/src/createDevMiddleware.js line 12 – Requires should be sorted alphabetically (lint/sort-imports)

Generated by 🚫 dangerJS against 3c25924

[motiz88]

@byCedric's updates to this PR (exposing unstable_Device, unstable_InspectorProxy as top-level exports, adding an unstable config option to replace the InspectorProxy implementation) make perfect sense to me for React Native 0.73. I've asked @blakef to help with landing this and getting it picked into the next 0.73 RC.

For 0.74 onwards, can we be proactive about moving away from these unstable mechanisms, and addressing the underlying problems in a sustainable way? Specifically:

we need Hermes to stop crashing apps on any CDP message

If this is a bug in Hermes's CDP handler, let's just fix it (cc @dannysu) and put unit tests around it - this doesn't have to be blocked on shipping the entire new CDP stack, and doesn't warrant keeping around a monkey-patched version of InspectorProxy.

For the network inspector, we can only move off our extended proxy when the new native Hermes CDP backend is ready and we re-implemented the network inspector in the new system.

Let's derisk this. Adding a native network inspector inside RN is still an important goal for us, but it's likely the new CDP stack will ship without it at first. In that world, continuing to use a monkey-patched InspectorProxy is a reliability risk. Can we minimally port Expo's existing network inspector to a supported, tested API in InspectorProxy and stop relying on monkey-patching? (Note that InspectorProxy has an actual test suite now.) Even if that API is itself unstable_ and we still have the goal of migrating to a fully native solution.

[facebook-github-bot]

@blakef has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[github-actions]

This pull request was successfully merged by @gabrieldonadel in 1a61afd.

^{When will my fix make it into a release? | Upcoming Releases}

[gabrieldonadel]

Summary:

Recently, both metro-inspector-proxy(#39045) and @react-native-community/cli-plugin-metro(#38795) were moved to this repo and in the process of moving these packages, the exports field inside package.json was added, only exporting the index.js file.

The problem is that Expo CLI (and possibly other community packages) rely on functions and classes that are not exported in the index.js file, e.g. Importing the InspectorProxy class from @react-native/dev-middleware/dist/inspector-proxy/InspectorProxy. Normally this wouldn't be a problem and we would just import from dist/ but due to the exports field, attempting to import from any other file not specified on this field will result in a ERR_PACKAGE_PATH_NOT_EXPORTED error.

As a short-term fix, we should create unstable_-prefixed exports of individual features Expo currently depends on.

Changelog:

[INTERNAL] [CHANGED] - Expose unstable_InspectorProxy and unstable_Device from @react-native/dev-middleware

Test Plan:

N / A

[motiz88]

Hey @gabrieldonadel, let's talk about alternatives to this PR. Generally speaking, strengthening the encapsulation of these packages was an intentional change to allow us to iterate faster, and removing exports would undo that.

Expo CLI should have a plan for moving off its dependencies on the internals of these packages, unless we make them intentionally public. Short term, we can be pragmatic here and create unstable_-prefixed exports of individual features Expo currently depends on, but you should expect them to be removed or otherwise break in a future release.

You mentioned InspectorProxy specifically. Note that we're actively working on this class and considering it an implementation detail of @react-native/dev-middleware. It's my understanding that in the long term, Expo CLI will use @react-native/dev-middleware directly and not depend on customising InspectorProxy. (cc @byCedric, @huntie)

Can you elaborate on other specific use cases for deep imports from these packages, and the plan for moving away from them?

[EvanBacon]

For all dev tools packages (minus CLIs), I'd prefer if we could reach the internals as the iteration cycles would cause us potentially weeks/months of delay to get changes out to users, which would cause us to either delay react native upgrades or need to fork more code away from the community packages.

Since debugging is fairly stable in Expo CLI at the moment and we're currently in the middle of the react-native upgrade process, we could update this PR to expose all of the internal APIs we're using, either through the main export or package exports.

[byCedric]

I can speak for the use case of customizing the InspectorProxy and Device classes. We currently extend these two classes to add functionality, mainly in two categories:

• Add new functionality, like the Network inspector.
• Add workarounds/fixes to avoid hard crashes with Hermes.

For the network inspector, we can only move off our extended proxy when the new native Hermes CDP backend is ready and we re-implemented the network inspector in the new system.

For the workaround / fixes, we need Hermes to stop crashing apps on any CDP message. Even when the CDP message content is malformed. E.g. Runtime.callFunctionOn could cause a crash when injecting a JS script using features not available in Hermes (see expo/vscode-expo#231 and expo/expo#25270 for the latest example, and all other Vscode... handlers to make vscode CDP messages not crash the app)

I think the best path forward on our end would be to keep extending these classes until the newer Hermes CDP backend is in place, and we migrated or upstreamed the network inspector.

For now, I think having access to the Device and InspectorProxy class, and it's initialization is the in-between step. If we could use createDevMiddleware with our own InspectorProxy class, we can switch to this in-between step and start consuming the @react-native/dev-middleware package. I believe this requirement around the InspectorProxy class was also what @huntie and I have discussed earlier, but correct me if I'm wrong 😄

[gabrieldonadel]

Regarding @react-native/community-cli-plugin we will no longer need to change it as we were only using the saveAssets function and Evan just opened a PR updating the CLI to no longer use it expo/expo#25278

[github-actions]

	Warnings
⚠️	packages/dev-middleware/src/createDevMiddleware.js#L12 - packages/dev-middleware/src/createDevMiddleware.js line 12 – Requires should be sorted alphabetically (lint/sort-imports)

Generated by 🚫 dangerJS against 3c25924

[motiz88]

@byCedric's updates to this PR (exposing unstable_Device, unstable_InspectorProxy as top-level exports, adding an unstable config option to replace the InspectorProxy implementation) make perfect sense to me for React Native 0.73. I've asked @blakef to help with landing this and getting it picked into the next 0.73 RC.

For 0.74 onwards, can we be proactive about moving away from these unstable mechanisms, and addressing the underlying problems in a sustainable way? Specifically:

we need Hermes to stop crashing apps on any CDP message

If this is a bug in Hermes's CDP handler, let's just fix it (cc @dannysu) and put unit tests around it - this doesn't have to be blocked on shipping the entire new CDP stack, and doesn't warrant keeping around a monkey-patched version of InspectorProxy.

For the network inspector, we can only move off our extended proxy when the new native Hermes CDP backend is ready and we re-implemented the network inspector in the new system.

Let's derisk this. Adding a native network inspector inside RN is still an important goal for us, but it's likely the new CDP stack will ship without it at first. In that world, continuing to use a monkey-patched InspectorProxy is a reliability risk. Can we minimally port Expo's existing network inspector to a supported, tested API in InspectorProxy and stop relying on monkey-patching? (Note that InspectorProxy has an actual test suite now.) Even if that API is itself unstable_ and we still have the goal of migrating to a fully native solution.

[facebook-github-bot]

@blakef has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[github-actions]

This pull request was successfully merged by @gabrieldonadel in 1a61afd.

^{When will my fix make it into a release? | Upcoming Releases}

[gabrieldonadel]

Summary:

Recently, both metro-inspector-proxy(#39045) and @react-native-community/cli-plugin-metro(#38795) were moved to this repo and in the process of moving these packages, the exports field inside package.json was added, only exporting the index.js file.

The problem is that Expo CLI (and possibly other community packages) rely on functions and classes that are not exported in the index.js file, e.g. Importing the InspectorProxy class from @react-native/dev-middleware/dist/inspector-proxy/InspectorProxy. Normally this wouldn't be a problem and we would just import from dist/ but due to the exports field, attempting to import from any other file not specified on this field will result in a ERR_PACKAGE_PATH_NOT_EXPORTED error.

As a short-term fix, we should create unstable_-prefixed exports of individual features Expo currently depends on.

Changelog:

[INTERNAL] [CHANGED] - Expose unstable_InspectorProxy and unstable_Device from @react-native/dev-middleware

Test Plan:

N / A

[motiz88]

Hey @gabrieldonadel, let's talk about alternatives to this PR. Generally speaking, strengthening the encapsulation of these packages was an intentional change to allow us to iterate faster, and removing exports would undo that.

Expo CLI should have a plan for moving off its dependencies on the internals of these packages, unless we make them intentionally public. Short term, we can be pragmatic here and create unstable_-prefixed exports of individual features Expo currently depends on, but you should expect them to be removed or otherwise break in a future release.

You mentioned InspectorProxy specifically. Note that we're actively working on this class and considering it an implementation detail of @react-native/dev-middleware. It's my understanding that in the long term, Expo CLI will use @react-native/dev-middleware directly and not depend on customising InspectorProxy. (cc @byCedric, @huntie)

Can you elaborate on other specific use cases for deep imports from these packages, and the plan for moving away from them?

[EvanBacon]

For all dev tools packages (minus CLIs), I'd prefer if we could reach the internals as the iteration cycles would cause us potentially weeks/months of delay to get changes out to users, which would cause us to either delay react native upgrades or need to fork more code away from the community packages.

Since debugging is fairly stable in Expo CLI at the moment and we're currently in the middle of the react-native upgrade process, we could update this PR to expose all of the internal APIs we're using, either through the main export or package exports.

[byCedric]

I can speak for the use case of customizing the InspectorProxy and Device classes. We currently extend these two classes to add functionality, mainly in two categories:

• Add new functionality, like the Network inspector.
• Add workarounds/fixes to avoid hard crashes with Hermes.

For the network inspector, we can only move off our extended proxy when the new native Hermes CDP backend is ready and we re-implemented the network inspector in the new system.

For the workaround / fixes, we need Hermes to stop crashing apps on any CDP message. Even when the CDP message content is malformed. E.g. Runtime.callFunctionOn could cause a crash when injecting a JS script using features not available in Hermes (see expo/vscode-expo#231 and expo/expo#25270 for the latest example, and all other Vscode... handlers to make vscode CDP messages not crash the app)

I think the best path forward on our end would be to keep extending these classes until the newer Hermes CDP backend is in place, and we migrated or upstreamed the network inspector.

For now, I think having access to the Device and InspectorProxy class, and it's initialization is the in-between step. If we could use createDevMiddleware with our own InspectorProxy class, we can switch to this in-between step and start consuming the @react-native/dev-middleware package. I believe this requirement around the InspectorProxy class was also what @huntie and I have discussed earlier, but correct me if I'm wrong 😄

[gabrieldonadel]

Regarding @react-native/community-cli-plugin we will no longer need to change it as we were only using the saveAssets function and Evan just opened a PR updating the CLI to no longer use it expo/expo#25278

[github-actions]

	Warnings
⚠️	packages/dev-middleware/src/createDevMiddleware.js#L12 - packages/dev-middleware/src/createDevMiddleware.js line 12 – Requires should be sorted alphabetically (lint/sort-imports)

Generated by 🚫 dangerJS against 3c25924

[motiz88]

@byCedric's updates to this PR (exposing unstable_Device, unstable_InspectorProxy as top-level exports, adding an unstable config option to replace the InspectorProxy implementation) make perfect sense to me for React Native 0.73. I've asked @blakef to help with landing this and getting it picked into the next 0.73 RC.

For 0.74 onwards, can we be proactive about moving away from these unstable mechanisms, and addressing the underlying problems in a sustainable way? Specifically:

we need Hermes to stop crashing apps on any CDP message

If this is a bug in Hermes's CDP handler, let's just fix it (cc @dannysu) and put unit tests around it - this doesn't have to be blocked on shipping the entire new CDP stack, and doesn't warrant keeping around a monkey-patched version of InspectorProxy.

For the network inspector, we can only move off our extended proxy when the new native Hermes CDP backend is ready and we re-implemented the network inspector in the new system.

Let's derisk this. Adding a native network inspector inside RN is still an important goal for us, but it's likely the new CDP stack will ship without it at first. In that world, continuing to use a monkey-patched InspectorProxy is a reliability risk. Can we minimally port Expo's existing network inspector to a supported, tested API in InspectorProxy and stop relying on monkey-patching? (Note that InspectorProxy has an actual test suite now.) Even if that API is itself unstable_ and we still have the goal of migrating to a fully native solution.

[facebook-github-bot]

@blakef has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[github-actions]

This pull request was successfully merged by @gabrieldonadel in 1a61afd.

^{When will my fix make it into a release? | Upcoming Releases}