-
-
Notifications
You must be signed in to change notification settings - Fork 218
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add metro
plugin
#895
Add metro
plugin
#895
Conversation
commit: |
|
||
const title = 'Metro'; | ||
|
||
const enablers = [/^metro(-.*)?$/, 'react-native']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is the best way to handle this, but typically Metro is not in the project's package.json, as it's a dependency of React Native
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, let's just use only react-native
? Is that ever not listed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@webpro It's definitely possible to have a project without react-native
listed. Metro can also bundle for web, although it'd be pretty rare to use it for exclusively that purpose over other web bundlers. Or the project could be a custom dev server that uses Metro's bundling API. Again, a rare use case, but possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can/should obviously leave some metro
specific enabler(s) here, but this regex matches anything that starts with metro
which is too loose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, I'll remove the regex in favor of 'metro'
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Jonathan, this is great!
The reason I held off introducing this plugin before is that it's not trivial to properly support Metro's module resolution, i.e. support the various *.{android,ios,web}.ts
etc. files. It basically means added complexity and creating the graph for each target separately (also see #126 if you haven't already).
How are you handling this in your project(s)?
There's a similar situation with the Nuxt plugin, which might lead to (lots of) false positives/confusion. We could add a note for this plugin too, should probably stand out more (not necessarily in this PR though).
I think overall this is definitely worth merging, I just like to also make sure users understand what "works with RN/Metro" means and how they should reason/go about it.
|
||
const title = 'Metro'; | ||
|
||
const enablers = [/^metro(-.*)?$/, 'react-native']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, let's just use only react-native
? Is that ever not listed?
@webpro Thanks for the review! I have looked through #126 in the past and I totally respect that it would add a lot of complexity to Knip. I don't have a lot of platform-specific extensions in my projects, but I have experimented with a little script that simply finds files with platform-specific extensions, and if they have a matching file without a platform-specific extension, it adds the file to my For me personally, there's a great benefit to a simple plugin to add entry points and dependencies even without the platform-specific extensions support. If a note about RN module resolution/platform-specific extensions is added to the docs, I'd suggest adding it for the Expo plugin too. Metro is a little more technical and new RN users will likely encounter Expo first (it's the recommended way to get started with RN from the RN docs). |
That's cool but if we market Knip as "supports React Native" I'd expect users in RN projects to run Knip and then face a ton of unused (platform-specific) files reported. For them I need a clear path out (tbh actually it's already too late for a good first impression then, but that's the risk I might be willing to take). |
@webpro I understand; it definitely makes sense from a "good first impression" perspective. Let me know if I can help move things along in this PR or another! |
Yeah not that Knip is perfect by any means, but I can easily imagine situations where many false positives might be overwhelming for new users. Was thinking we could add a note to the plugin page and whip up a page dedicated to RN, perhaps a bit like https://knip.dev/features/integrated-monorepos. Maybe configuration hints directly in the CLI output. If you could make this PR green then we can wrap this up soon I guess :) |
Thanks Jonathan! Planning to prepare a release in the coming days. |
Tried a few projects and using those default production entry patterns work pretty well: knip/packages/knip/src/plugins/metro/index.ts Lines 23 to 41 in 093dcca
E.g. in https://github.com/pass-culture/pass-culture-app-native I didn't need to change the defaults, in others there's only a small number of false positives. With the simple plugin docs hopefully it's pretty clear what's going on: https://4f7938f0.knip.pages.dev/reference/plugins/metro If you'd like to try it out install with something like In case you have any concerns or feedback I'd love to hear about it :) |
E.g. in https://github.com/Expensify/App this override seems to work well: {
"metro": {
"entry": ["src/**/*.{ios,android,windows,web,native,default,desktop,website}.{js,jsx,json,ts,tsx}"],
},
} |
🚀 This pull request is included in v5.42.0. See Release 5.42.0 for release notes. Using Knip in a commercial project? Please consider becoming a sponsor. |
This adds a plugin for Metro, the JavaScript bundler for React Native.