Doesn't resolve index.js files correctly

[jaydenseric]

Thanks for such an awesome tool!

Our project had a src/constants/index.js file with a bunch of named exports.

These named exports were imported in components within src/components with a specifier something like ../constants, and shrimpit was assuming this means ../constants.js and didn't bother looking for ./constants/index.js like Node.js and other tools do.

This resulted in all our constants reporting incorrectly as unused.

[yamafaktory]

Sorry for the late reply... missed this notification... anyway, yes this is a bug indeed and it should definitely handle that by default 👍 .

[jaydenseric]

I ended up writing from scratch and publishing find-unused-exports; A Node.js CLI and equivalent JS API to find unused ECMAScript module exports in a project.

It can resolve index files, custom extensions, etc. One thing I was not expecting was for it to be so fast — find-unused-exports takes less than a second for our project whereas Shrimpit for some reason takes quite a few minutes. It also supports ignoring specific exports known to be unused (e.g. the default export for a Next.js page) via ignore comments, making the tool fit for use as part of the test script for CI.

[jaydenseric]

I got started on a Shimpit PR that fixed most of the problems with index files, but gave up before getting every type of import/export to work. If you're interested, I might be able to share what I had in a draft PR.

[yamafaktory]

Sure, thanks!

[jaydenseric]

The strategy is to not attempt to guess the absolute import file paths at the time they are discovered, but to record the import specifiers more faithfully:

{
- "location": "test/core/b/b.js",
+ "location": "test/core/b/b",
   "name": "test",
   "unnamedDefault": true,
}

Then later when analysing what exports are unused, search for the files in the list, resolving file extensions and index files in order of preference (it's good to match the Node.js resolution algorithm).

Rather than do a fork and draft PR, here are the relevant diffs (minus updated tests and snapshots)…

shrimpit/lib.js

Line 282 in 6486693

const { exports, imports } = this.modules

     const { exports, imports } = this.modules
     const unresolved = exports.reduce((acc, item) => {
       if (
-        imports.filter(element =>
-          element.unnamedDefault
+        imports.filter(imprt =>
+          imprt.unnamedDefault
             ? // Skip the name in the comparison as a default unnamed export
               // can be imported with any name.
               // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/export
               this.deepStrictEqual(
-                { location: element.location, unnamedDefault: true },
+                { location: `${imprt.location}.js`, unnamedDefault: true },
                 {
                   location: item.location,
                   unnamedDefault: item.unnamedDefault,
                 },
               )
             : // Compare the raw element & item.
-              this.deepStrictEqual(element, item),
+              this.deepStrictEqual(
+                {
+                  ...imprt,
+                  location: `${imprt.location}.js`,
+                },
+                item,
+              ),
+        ).length === 0 &&
+        imports.filter(imprt =>
+          imprt.unnamedDefault
+            ? this.deepStrictEqual(
+                {
+                  location: `${imprt.location}${path.sep}index.js`,
+                  unnamedDefault: true,
+                },
+                {
+                  location: item.location,
+                  unnamedDefault: item.unnamedDefault,
+                },
+              )
+            : this.deepStrictEqual(
+                {
+                  ...imprt,
+                  location: `${imprt.location}${path.sep}index.js`,
+                },
+                item,
+              ),
         ).length === 0
       ) {
         acc.push(item)

That's not the best way to write it; this was just my work in progress to patch the way the code currently works.

shrimpit/lib.js

Line 410 in 6486693

const getLocation = location =>

     const getLocation = location =>
-      this.joinPaths(
-        this.getDir(extPath).join(path.sep),
-        location + this.getExt(extPath),
-      )
+      path.join(this.getDir(extPath).join(path.sep), location)

A side note: I avoided using this.joinPaths; it's better to path.join directly. Unhelpful methods like this could be removed to simplify the code and make it easier to understand what's going on:

shrimpit/lib.js

Lines 207 to 209 in 6486693

	joinPaths(...paths) {
	return path.join(...paths)
	}

I think namespace imports were not working at the time I abandoned the effort? I'm struggling to remember to be honest it was a few weeks ago.