Platform-specific extensions​ about knip HOT 25 OPEN

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 src/components/Header/index.android.ts and a src/components/Header/index.ios.ts file, and there is no src/components/Header/index.ts file.

Similarly, I could have src/components/Header/index.android.ts, and then also just use index.ts as the default option (this is really only a meaningful difference if you have more than two platforms, which many projects do).

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 <EXPECTED_PATH>/<NAME>.<ANYTHING OR NO EXTENSION>.js exist?"

Good support would be having a config option where we can specify allowed platform-specific extensions and check if at least one of <EXPECTED_PATH>/<NAME>.<PICK FROM ALLOWED_EXTENSIONS OR NO EXTENSION>.js exists.

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".

from knip.

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!

from knip.

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 resolution

I'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.

Plugin

At 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 @react-native/metro-config is in the list of dependencies. Is that enough?

Configuration

Since 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 metro.config.js file needs to have an extra export:

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 __KNIP_PLATFORMS__ (not final!) export will be picked up by Knip's Metro plugin. In this example:

• An import specifier like ./Component during the ios target run will look for/resolve to ./Component/index.ios.*, ./Component/index.native.*, ./Component/index.*, ./Component.ios.*, ./Component.android.* and ./Component.*.
• Mix and match however you need.
• The ios key is just a name that will display in the output (use anything you want).

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 config

There'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 knip.json configuration:

{
    "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 __KNIP_PLATFORMS__ in the metro.config.js as shown above.

This gave pretty good results. As far I can see, since it's not trivial to manually verify what some component.android.ts is actually not used.

Installation

Feel free to try it out:

npm install -D knip@metro

Feedback

Feel 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!

from knip.

I missed the important piece there. I tried out v0.0.0-metro.1 and there's a definite improvement.

from knip.

@alburdette619 Any chance you can share a repo or a reproduction? I can't see or debug anything without an actual codebase.

from knip.

Just following this, but to add:

The plugin is activated if @react-native/metro-config is in the list of dependencies. Is that enough?

Expo apps (an abstraction on top of React Native) would likely also benefit from this.

Please could you include @expo/metro-config alongside @react-native/metro-config?

from knip.

Just following this, but to add:

The plugin is activated if @react-native/metro-config is in the list of dependencies. Is that enough?

Expo apps (an abstraction on top of React Native) would likely also benefit from this.

Please could you include @expo/metro-config alongside @react-native/metro-config?

Sure. In the meantime, plugins can be force-enabled by setting e.g. metro: true in the Knip config.

Would be great if codebases could be tested, or shared so we can test things on those.

from knip.

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 .ts file, then import the type from a .android.ts file.
This is probably not good practice, tbh (a types.d.ts file would probably be more sensible), but it is valid and Typescript seems happy with it.

I'll find some time to put together a reproduction again.

from knip.

Fairly simple to reproduce the exports issue:

diff --git a/packages/knip/fixtures/plugins/metro/src/About.android.js b/packages/knip/fixtures/plugins/metro/src/About.android.js
index e69de29b..807a7975 100644
--- a/packages/knip/fixtures/plugins/metro/src/About.android.js
+++ b/packages/knip/fixtures/plugins/metro/src/About.android.js
@@ -0,0 +1 @@
+export default () => 1;
diff --git a/packages/knip/fixtures/plugins/metro/src/About.ios.js b/packages/knip/fixtures/plugins/metro/src/About.ios.js
index e69de29b..807a7975 100644
--- a/packages/knip/fixtures/plugins/metro/src/About.ios.js
+++ b/packages/knip/fixtures/plugins/metro/src/About.ios.js
@@ -0,0 +1 @@
+export default () => 1;
diff --git a/packages/knip/fixtures/plugins/metro/src/About.js b/packages/knip/fixtures/plugins/metro/src/About.js
index e69de29b..807a7975 100644
--- a/packages/knip/fixtures/plugins/metro/src/About.js
+++ b/packages/knip/fixtures/plugins/metro/src/About.js
@@ -0,0 +1 @@
+export default () => 1;

Knip reports that the default exports are unused.

from knip.

Oh you meant to provide a patch, gotcha:

Unused exports (2)
default  unknown  src/About.android.js:1:8
default  unknown  src/About.ios.js:1:8

I'll look into it. Still, it would be good to have more real-world repos to exercise Knip on.

from knip.

No worries, nothing personal, just gauging interest here :)

from knip.

Interesting. How could Knip know about such platform-specific conventions? Sounds like it's not a generic file name convention like index.ts, but rather any component file such as Button.tsx must have Button.android.tsx, right?

from knip.

@webpro thanks for your response. This works with any file, not only components. You can start with index.ts but then you realize MacOS requires a totally different logic so you create index.macos.ts, and then the bundler will pick either index.ts or index.macos.ts at build time.
I think config can be something like: "platformSpecificExtensions" with the default ["ios", "android", "windows", "macos", "web"]

from knip.

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.

from knip.

Yes, is react-native specific. There could be cases where there is no base file, and only platform-specific files exist. Eg. something.android.js and something.ios.js might exist but something.js don't. It's not common but it might happen. In that case, walking the file tree to see if the base file is used will not work because there's no base file

from knip.

Yes, that's what I meant: walk the file tree and look for something.[ios|android].js etc and see if something.js is there as well. If not, that's an issue. That's basically all it takes, right? In that case I'm not sure how it fits in what Knip is doing.

from knip.

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 Home.js file and Home.android.js, Home.ios.js and Home.web.js will be marked as unused even with the import in App.js:2

from knip.

I meant something like this (generated by chatpgt):

#!/bin/bash

find . -type f \( -name "*.android.js" -o -name "*.ios.js" \) | while read -r file; do
    base="${file%.*.*}"
    [[ ! -e "${base}.ts" ]] && echo "Missing ${base}.ts for $file"
done

from knip.

To re-iterate:

• Knip only considers imported files, the rest is reported as being unused
• The *.[platform].ts{,x} files can be ignored in Knip config
• Knip will report unused base files.
• A custom script (like in my latest comment) can then look for platform files without a base file to also clean them up.

from knip.

unimported just put their library into archive and are recommending this library. Using entry configs, I was able to get unimported to support this fully with dynamic something.xyz.ts syntax. See the issue I solved for myself here.

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.

from knip.

@webpro thanks, I'll give this a shot as soon as I can.

from knip.

@webpro It seems to work but with a few issues:

• It seems providing a lot of values to __KNIP_PLATFORMS__ starts to provide false positives. If I have android, ios, & xyz all is well and no .xyz files show up. If I add abc, def, & ghi suddenly there are .xyz files that are reported.
• I'm seeing a lot of issues that seem similar to #529 suddenly in a specific case. This probably isn't related, I cleared my yarn cache even & am on v5.6.0 (also tried on v5.1.1), but it wasn't there before somehow.
  • This was a multiple direct import/export (export { default as X } from '...') statements in one file that when restructured worked.

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 metro.config.js (extensions changed for anonymity):

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", ""],
  ],
};

from knip.

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.

from knip.

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:

❯ knip                     # or `tsx ../../../src/cli.ts` or `npx -y knip@metro`
Unused files (2)
src/About.web.js
src/Home.web.js
Unused dependencies (3)
@react-native/metro-config  package.json
expo                        package.json
metro-resolver              package.json
Unused devDependencies (1)
react-native  package.json
Unresolved imports (2)
./Header  src/main.js:1:19
./Home    src/main.js:4:17

from knip.

Thanks for trying and calling it out @ball-hayden, just published v0.0.0-metro.1 with a fix.

from knip.