import-js/eslint-plugin-import

eslint-plugin-import silently fails if the module is minified

RPGillespie6 opened this issue · 27 comments

I have 2 files:

  1. test.js
import { bogus } from './bogus.js';

bogus()
  1. bogus.js
function foo() {
    console.log('foo');
}

export {
    foo
}

If bogus.js is minified, eslint-plugin-import silently fails:

$ npx eslint js/test.js        # Working case

/home/user/test/js/test.js
  1:10  error  bogus not found in './bogus.js'  import/named

✖ 1 problem (1 error, 0 warnings)

$ npx minify js/bogus.js > bogus2.js
$ mv bogus2.js js/bogus.js
$ npx eslint js/test.js        # Silent failure case
$

Here's my eslint config:

import globals from "globals";
import pluginJs from "@eslint/js";
import importPlugin from "eslint-plugin-import";

/** @type {import('eslint').Linter.Config[]} */
export default [
  importPlugin.flatConfigs.recommended,
  {languageOptions: { globals: globals.browser }},
  pluginJs.configs.recommended,
];

Tested with eslint 8.X and 9.X, issue is present in both. Using eslint-plugin-import:2.31.0

Edit: Actually, some types of minification work. For example:

function a(){console.log("foo")};export{a}; // works
function a(){console.log("foo")}export{a}; // doesn't work

Is this library using regex or something to detect exports instead of the file's AST? That's the only thing I can think of that would make it so that the absence of an optional semicolon causes silent failures

That’s very strange. (altho minifying a package never makes sense, ever - minification is something that should only ever be done by a top-level app)

I’ll look into it.

Thanks, let me know if you can't repro, but I was able to repro in a fresh project with the latest eslint and eslint-plugin-import

altho minifying a package never makes sense, ever - minification is something that should only ever be done by a top-level app

Not sure what you mean by this. In my case it's a TypeScript file that is compiled to a minifed JS esmodule, which is then distributed for use by legacy non-typescript esmodules. We use this plugin to ensure the imports using that esmodule are correct.

Why would it need to be minified, though? Transpiling is fine, but minification is something else altogether.

Why would it need to be minified, though? Transpiling is fine, but minification is something else altogether.

So that devs don't attempt to modify it. It's a hint to the developer that it's a build artifact of upstream source, and that they should be modifying the upstream source.

This is a very standard and common practice for distributing libraries. For example: https://pixijs.download/v8.5.2/pixi.min.mjs

This is PixiJS as an esmodule. If I'm using this in my project, I would hope:

import { bogus } from "./pixi.min.mjs"

Would get flagged by eslint-plugin-import because that's not a valid symbol exported by pixijs

This is a very standard and common practice for distributing libraries.

It's common for libraries that are generally consumed directly through html / browser scripts (via cdn downloads), since it reduces download size / time. But I would assert that it's an antipattern for libraries that are consumed through package managers and ultimately built into an app. It negatively impacts developer experience. Shit happens and people need to debug code, and if you're trying to figure out what a library is doing to investigate an issue, having it minified is a huge pain in the ass, with no real benefit.

Why would a developer ever be modifying third-party code? If they are willing to do that in the first place, then they're already violating every best practice that exists, and no hint is going to stop them.

For distributing libraries as a drop-in script tag, yes, but that's archaic and obsolete and from the earlier part of the past decade. npm packages are never minified.

Can you provide the minified content of bogus.js, so i can make a test case?

Not sure why you are getting so defensive, no offense was intended. Minified javascript is valid javascript, we aren't using npm to manage any front end dependencies, just a few dev tools like eslint and esbuild (to strip types out of TS, which we then load directly as an ESM). You would open an issue against a json parser that couldn't parse minified json, wouldn't you?

Can you provide the minified content of bogus.js, so i can make a test case?

function foo(){console.log("foo")}export{foo};

Edit:

Also note some minifiers also do obfuscation as well, so you could end with something like:

function a(){console.log('foo')}export{a as foo};

(though in my case, I'm not obfuscating, just removing whitespace)

Indeed, it's a bug, as I indicated previously, but it's always important to try to stop bad practices whenever they appear, and minifying a published package is one such bad practice :-)

I'm also very confused; this isn't a published package it seems, but a relative file - why would you be linting build output of any kind? In your OP I would expect both test.js and bogus.js to be the unminified untranspiled raw output, since they're both in the project. (it's relevant because I'm trying to set up the reproduction test case)

In this case it's a legacy codebase that was created in 2016. When it was first created it was an AngularJS app. Then Google abruptly killed AngularJS, and in our wrath we swore off JS frameworks in favor of vanilla esmodules, vanilla web components, etc. ("frameworks are faddish, but vanilla is forever"). So new pages started just using vanilla esmodules like <script type="module" src="/js/account.mjs">

This seemed to work well and is very simple. No dependency on npm, no build steps, no file watchers, no bundlers. Write modular code, but just hit refresh to pick up any changes. Now of course, over time, we kept getting bitten by JS's lack of typing and so we wanted to start using TypeScript. However, there are a lot of .mjs files, so it wasn't feasible to convert them to TS all at once. So we are converting them to TS piecemeal as we have bandwidth. But that causes a problem. If we convert a shared module, such as utils.mjs, to TS, then the .mjs files that haven't been convert to TS yet can't use it.

So we introduced esbuild to convert TS to JS so that the .mjs files that haven't been converted yet can still use the module. However, now we have a situation where it's easy to accidentally make a change to the wrong source file (TS vs JS), and so we minify the JS so that it's clear that you should not modify it if you need to make a change.

Eventually, all the mjs files will be converted to TS and we will no longer need eslint-plugin-import because tsc will check all the imports instead. But right now we are in a migratory period where half the modules are TS and half aren't, but we still want to be sure that we haven't messed up any imports on the ESMs that are still JS.

We have not yet bitten the bullet on going all-in on adding npm to the critical path. Currently npm is not in the critical path at all. The .mjs files derived from the TS are committed to the repo. This is primarily to accommodate a few non-developers that occasionally clone the repo, check out a branch, start the webserver, and then perform QA.

I'm not claiming this system is perfect, but I think it's a valid design decision to avoid the baggage that comes with npm, since it introduces quite a bit of complexity, maintenance burden, and risk to the system.