fix(expo): VS code extension not picking up expo support via metadata in package.json#8980
fix(expo): VS code extension not picking up expo support via metadata in package.json#8980russellwheatley wants to merge 1 commit intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Code Review
This pull request adds Expo plugin configurations to the package.json files of several packages, including analytics, app, app-check, app-distribution, auth, crashlytics, messaging, and perf. It also introduces a new test suite to verify that these packages correctly declare the Expo plugin and that the corresponding app.plugin.js files exist. The review feedback suggests refining the exports type definition in the test to support conditional exports, removing redundant file existence checks, and dynamically discovering packages within the test suite to improve maintainability.
| expo?: { | ||
| plugin?: string; | ||
| }; | ||
| exports?: Record<string, string>; |
There was a problem hiding this comment.
The exports field in package.json can contain nested objects for conditional exports (e.g., import, require, default), not just strings. The current type definition is too restrictive and technically incorrect for packages like @react-native-firebase/app which use an object for the . export.
| exports?: Record<string, string>; | |
| exports?: Record<string, string | Record<string, unknown>>; |
| const packageJsonPath = path.resolve(repoRoot, 'packages', pkg, 'package.json'); | ||
| const pluginEntryPath = path.resolve(repoRoot, 'packages', pkg, 'app.plugin.js'); | ||
|
|
||
| expect(fs.existsSync(packageJsonPath)).toBe(true); | ||
| expect(fs.existsSync(pluginEntryPath)).toBe(true); |
There was a problem hiding this comment.
The packageJsonPath is already constructed and used inside the readPackageJson function. You can simplify this test by removing the redundant path calculation and the explicit existsSync check for the manifest, as readPackageJson handles file reading and error reporting internally.
| const packageJsonPath = path.resolve(repoRoot, 'packages', pkg, 'package.json'); | |
| const pluginEntryPath = path.resolve(repoRoot, 'packages', pkg, 'app.plugin.js'); | |
| expect(fs.existsSync(packageJsonPath)).toBe(true); | |
| expect(fs.existsSync(pluginEntryPath)).toBe(true); | |
| const pluginEntryPath = path.resolve(repoRoot, 'packages', pkg, 'app.plugin.js'); | |
| expect(fs.existsSync(pluginEntryPath)).toBe(true); |
| const pluginPackages = [ | ||
| 'analytics', | ||
| 'app', | ||
| 'app-check', | ||
| 'app-distribution', | ||
| 'auth', | ||
| 'crashlytics', | ||
| 'messaging', | ||
| 'perf', | ||
| ] as const; |
There was a problem hiding this comment.
The list of packages is currently hardcoded. To make this regression test more robust and ensure it automatically covers any future packages that might include an Expo plugin, consider dynamically discovering the packages by scanning the packages/ directory for the presence of an app.plugin.js file.
Description
Expo config plugin tooling could warn that RNFB packages were not valid config plugins when referenced by package name alone, even though
app.plugin.jswas present and direct references to that file worked. This change makes the plugin entry explicit by adding Expo plugin metadata to every RNFB package that shipsapp.plugin.js, and adds a regression test to keep that manifest contract in place.possibly closes #8976
Await user feedback.
Related issues
Release Summary
Checklist
AndroidiOSOther(macOS, web)e2etests added or updated inpackages/\*\*/e2ejesttests added or updated inpackages/\*\*/__tests__Test Plan
Think
react-native-firebaseis great? Please consider supporting the project with any of the below:React Native FirebaseandInvertaseon Twitter