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
Platform-specific extensions #126
Comments
Interesting. How could Knip know about such platform-specific conventions? Sounds like it's not a generic file name convention like |
@webpro thanks for your response. This works with any file, not only components. You can start with |
This is all specific to React Native, right? And any "base" source file could have one or more platform-specific "sibling" files? This isn't about calculating the dependency tree and then see what's unused. It's more the other way around: see what unused files are actually used. Kind of feel this isn't something Knip should do. At this point I think it's better/easier to have a separate script walk the file tree and check whether any platform-specific file has an accompanying "base" file. FYI, Knip plugins add project and entry files, compilers add non-JS/TS files, these are different things than what's requested. |
Yes, is react-native specific. There could be cases where there is no base file, and only platform-specific files exist. Eg. |
Yes, that's what I meant: walk the file tree and look for |
I'm still having doubts about the case mentioned above when the base file doesn't exist. I can't see how your approach works in that case For example, in this case, there's no |
I meant something like this (generated by chatpgt):
|
To re-iterate:
|
I think the behavior of platform-specific extensions did not get communicated properly here. "Base files" do not necessarily exist when using platform-specific extensions. I can have a Similarly, I could have This is controlled by the Metro JS server configuration: https://facebook.github.io/metro/docs/configuration#platforms. The React Native template currently configures Metro to only use the "android" and "ios" platforms. This gets more complicated when using react-native-web. People will often use webpack to serve the JS locally for web, and then configure it to be aware of platform-specific extensions. The most widely-used configuration for this would probably be Expo's. A good open-source example project to look at would be the Expensify app (https://github.com/Expensify/App/), which has mobile, web, and desktop platforms (using Electron and React Native Web), and heavily uses platform-specific extensions. tl;dr: What we need for React Native to have decent support with Knip would be to check "Does any file matching Good support would be having a config option where we can specify allowed platform-specific extensions and check if at least one of It's debatable whether Knip should verify if the platform-specific extension set should be "complete" or not (i.e., you have a file for each platform, OR you have some platform-specific files and a fallback with no platform-specific extension), because it could cause a crash if you ever reach that code path on a non-implemented platform, but that won't necessarily happen if your code is implemented to conditionally use that file based on platform. There's an extra layer of complexity in understanding the semantics of the "native" extension, which means "both iOS and Android". |
This is the primary thing keeping us from trying out and potentially recommending Knip on our projects (since we only do React Native). Like the project though! |
unimported just put their library into archive and are recommending this library. Using As iterated several times here, the script above does not handle all cases. We specifically have white labled apps that may or may not have a base file. I'll try the recommendations above to see how much of my use case it will handle. |
So, I took a stab at it. Thanks a lot @lindboe and @neiker for the details. The implementation of metro-resolver was useful too. The challenge here is that it's not just about extending entry files with some extensions For good results we actually need to walk the tree again for each target platform. Module resolutionI've created a mechanism in Knip so plugins can "inject" module resolvers and resolve as we need it. It's not final yet, but this gives a lot of freedom without complicating the core of Knip too much. Also, the module resolver implementation and (performance) improvements can stay entirely in the plugin. Initially I tried to use metro-resolver directly, but unfortunately hit some roadblocks. Ideally, Knip eventually will have some optimized version of that implementation. This very first alpha release uses the TS module resolver (in a rather inefficient way), but it seems to do the job. PluginAt this point I feel like it's more a Metro plugin than a React Native plugin, so that's what we introduce here. But feel free to change my mind on this. The plugin is activated if ConfigurationSince I've used the Expensify/App repo as an exercise for the new plugin, that also told me it's not trivial to auto-detect the platforms and desired module resolutions. Please bear with me here: for now, just for starters, the const config = {
resolver: {
assetExts: defaultAssetExts,
sourceExts: [...defaultSourceExts, 'jsx'],
},
};
module.exports = mergeConfig(defaultConfig, config);
module.exports.__KNIP_PLATFORMS__ = {
ios: [
['/index', ''],
['.ios', '.native', ''],
],
android: [
['/index', ''],
['.android', '.native', ''],
],
desktop: [
['/index', ''],
['.desktop', ''],
],
website: [
['/index', ''],
['.website', ''],
],
}; This
It ain't very pretty. The goal is to get rid of it entirely and auto-detect as much as possible (with the option to extend/override). Also see the https://github.com/webpro/knip/tree/feat/react-native-metro/packages/knip/fixtures/plugins/metro fixture and test in that branch. Expensify configThere's a fixture and a test in the Knip repo, and the plugin has been tested on the Expensify/App repo as well using this {
"entry": [
"src/pages/**/*.{js,ts,tsx}",
"tests/perf-test/**/*.perf-test.*",
"workflow_tests/*.test.js",
"src/libs/E2E/reactNativeLaunchingTest.ts"
],
"webpack": "config/webpack/webpack.*.js",
"ignore": ["**/__mocks__/**", "workflow_tests/mocks/**"],
"ignoreDependencies": ["@assets/*"]
} And by adding the This gave pretty good results. As far I can see, since it's not trivial to manually verify what some InstallationFeel free to try it out:
FeedbackFeel free to suggest radical changes, this is really just an early alpha version. Happy to receive any of your feedback in this issue. Please file detailed bugs in separate tickets. It definitely helps if you'd configure and run it in your project(s) already! |
@webpro thanks, I'll give this a shot as soon as I can. |
@webpro It seems to work but with a few issues:
My context again is that my app has a few white labels and uses this profile extension in a custom way to compile specific experiences for these white labels. In my module.exports.__KNIP_PLATFORMS__ = {
// Platforms
android: [
["/index", ""],
[".android", ""],
],
ios: [
["/index", ""],
[".ios", ""],
],
// Whitelabels
xyz: [
["/index", ""],
[".xyz", ""],
],
abc: [
["/index", ""],
[".abc", ""],
],
def: [
["/index", ""],
[".def", ".de", ""],
],
deg: [
["/index", ""],
[".deg", ".de", ""],
],
hij: [
["/index", ""],
[".hij", ""],
],
}; |
@alburdette619 Any chance you can share a repo or a reproduction? I can't see or debug anything without an actual codebase. |
Just following this, but to add:
Expo apps (an abstraction on top of React Native) would likely also benefit from this. Please could you include |
Sure. In the meantime, plugins can be force-enabled by setting e.g. Would be great if codebases could be tested, or shared so we can test things on those. |
Thank you. I'm seeing similar issues to @alburdette619. I'll have a play around with the fixture file to see if I can get a reproduction together. |
Fairly simple to reproduce the exports issue:
Knip reports that the default exports are unused. |
Not sure I understand this output. Not seeing any unused exports here:
|
Oh you meant to provide a patch, gotcha:
I'll look into it. Still, it would be good to have more real-world repos to exercise Knip on. |
Thanks for trying and calling it out @ball-hayden, just published |
Sorry - I should have made that clear it was a patch. That seemed like the easiest way to describe what I was seeing. And sorry also that I can't give you access to our repo - I really would like to. We've a couple of places where we export a type from the I'll find some time to put together a reproduction again. |
No worries, nothing personal, just gauging interest here :) |
I missed the important piece there. I tried out |
Hey! I'm using knip with react-native and after some adjustments it works wonderfully except for one detail. I can't find how to make files with platform-specific extensions be seen as the same files by knip.
Eg. I have a file
Component.tsx
and in the same path aComponent.android.tsx
, the first is detected as used but the second doesn't, when in fact they should be considered "the same file".As a workaround, I added
"**/*.android.ts{,x}"
onignore
but in caseComponent.tsx
is removed someone might forget to removeComponent.android.tsx
and knip will not detect itThe text was updated successfully, but these errors were encountered: