Skip to content
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

eslint-plugin-import breaks eslint if no package.json-file is present #2096

Open
manuth opened this issue May 24, 2021 · 27 comments
Open

eslint-plugin-import breaks eslint if no package.json-file is present #2096

manuth opened this issue May 24, 2021 · 27 comments

Comments

@manuth
Copy link
Contributor

manuth commented May 24, 2021

If there is no package.json-file in the file to check (or any parent directory), the eslint-run fails due to this piece of code:
https://github.com/benmosher/eslint-plugin-import/blob/802ce7d49d912289d590f735bf9bd2d931064863/src/core/packagePath.js#L11-L12

Following error is reported:

TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received null
Occurred while linting {filename}.js
@ljharb
Copy link
Member

ljharb commented May 24, 2021

We should definitely have a better error message - but why would you ever be running eslint outside of a node project with a package.json? global installs and personal configs are deprecated.

@manuth
Copy link
Contributor Author

manuth commented May 24, 2021

I had it running without a package.json for testing the validity of my default .eslintrc.js-settings.

In order to do so, I create temporary .js or .ts-files and pass the corresponding eslint-settings to test programmatically rather than through a .eslintrc-file.

I think this is pretty much an edge-case. My current solution is to create a package.json file in the same directory like the test-files.

@ljharb
Copy link
Member

ljharb commented May 24, 2021

It's certainly a scenario that's incompatible with this plugin.

We should improve the error message here, though.

manuth added a commit to manuth/ESLintPresets that referenced this issue May 24, 2021
Currently, `eslint-plugin-import` doesn't support workspaces
without a `package.json`-file in it.
The issue describing this matter is import-js/eslint-plugin-import#2096
@g1thuser
Copy link

We should definitely have a better error message - but why would you ever be running eslint outside of a node project with a package.json? global installs and personal configs are deprecated.

I also don't have a package.json, just because I'm not deploying any node modules in my project and therefore I simply don't need a package.json. IMHO this should be considered as a bug as it worked before without a package.json.

@GiladShoham
Copy link

Hi,
I'm the maintainer of https://github.com/teambit/bit
Many of our users have workspaces without a package.json file (as bit resolve dependencies from another file).
This breaking eslint entirely is a real issue for us.
I wish to have a way to tell the plugin to only check by other strategies (like making sure the package exists in the node_modules) folder, but skip the package.json validation completely (then also not throw an error in that case).

@ljharb
Copy link
Member

ljharb commented Aug 31, 2021

@GiladShoham I'm a bit unclear how any workspace workflow could function without package.json (in a JS project); lerna, yarn, npm, rush, etc all use package.json for their workspaces implementation; even bazel has one in the root by default for JS projects. Things inside node_modules always have package.json, as far as I'm aware, since node_modules is not for manual modification, and all the tools that modify it require a package.json to be in the package (to provide the "name", if nothing else)

@dapullar
Copy link

dapullar commented Sep 5, 2021

@ljharb I'm experiencing similar issues under bazel. Although the root of the project may contain a package.json, the environment where the linting takes place may not. Since everything is sandboxed in bazel the package.json would need to be explicitly added to each lint target. You can almost think of it as a docker container which only includes the files to be linted + eslint (binary & plugins) per defined unit.

I think the requirement of a package.json is fine but, the error message threw me for a loop, coupled with a long debugging session to realize the missing dependency.

@GiladShoham
Copy link

@ljharb Using bit, you don't have a package.json in your workspace (only with your final packages). but the lint environment is a workspace. which means you don't have a package.json.
Bit manages all the dependencies using a file called workspace.jsonc which supports many additional features for dependencies management.
Using bit install command, bit will calculate the final dependencies for your workspace, and programmatically use a package manager (like yarn / npm / pnpm) to install them.
So the best option is somehow to teach the plugin about bit's workspace.jsonc file or even better to bit's final calculated dependencies.
But as I guess this won't be easy / high priority on your side. at least an option to not check the package.json but only the node_modules folder will be really great.
Anyway, the error when there is no pacakge.json for sure should be more descriptive (regardless of bit)

@ljharb
Copy link
Member

ljharb commented Sep 9, 2021

@GiladShoham the pattern you describe will break all tooling in the ecosystem that expects package.json to be present - in other words, you can only lint a node project, and it's not a node project unless there's a package.json.

The best option is to fix bit to follow node conventions instead of radically deviating from them.

Certainly the error message can be made more descriptive.

@g1thuser
Copy link

@ljharb sorry, but I completely disagree. The purpose of the package.json is to create a npm package to share a project. If you don't want to share your project, you don't need a npm package and therefore also no package.json.

Furthermore eslint is for linting any javascript code, not only javascript code which is part of a npm package. So why does this restriction exist for eslint-plugin-import? I'm using several other third party plugins and not one of them is making this restriction.

And this has also nothing to do with not following node conventions. There aren't any node conventions which force you to create a npm package. Your arguments are not convincing and I'm still having the opinion, that this behavior should be considered as a bug.

@dapullar
Copy link

dapullar commented Sep 11, 2021

I agree with @g1thuser on this. eslint (& it's plugins) should always be functional as a standalone binary & should be able to lint the given files without external dependencies. If an external dependency is introduced (eg. package.json) it should be overridable in some manner. A linter should not rely on a specific workspace to be able to function without opt-in/out strategies.

At the end of the day we are sorting imports* here, you will be hard-pressed to convince everyone that a sorting algo requires a node project structure.

* I know more than sorting is going on, just being a little facetious

@GiladShoham
Copy link

I agree with @g1thuser
Actually, you don't need a package.json to run a node project. you might need a package.json inside the packages in the node_modules folder, but not on the root.
You can even have node_modules and then delete the package.json in the root and everything will run and work just fine without any issue.

Changing bit is a discussion that can be done, but it's not really related to this issue (I don't see it changes for many reasons).

The main issue is that a plugin completely break eslint of a result of not having the package.json which is not a must.
If someone wants this behavior I think it should be opt-in (maybe even a different rule that validate that there is a package.json)
Or at least, as it's not a must, there should be a way to opt-out from this behaviour of breaking lint completely.

@ljharb
Copy link
Member

ljharb commented Sep 14, 2021

@g1thuser that's not the only purpose of package.json. node itself uses it even for non-published apps for a few purposes. There ABSOLUTELY are node conventions that force a package.json, in other words.

You're welcome to opt out by not using this plugin, but javascript without a package.json is JS that's stuck in the early part of the previous decade, so I'm not concerned with linting that. In particular, eslint-plugin-import doesn't apply for anything except node, and only recently, ESM - but we don't truly yet support native ESM, only transpiled ESM, so there's not yet a use case to omit package.json for this plugin.

@dapullar
Copy link

You're welcome to opt out by not using this plugin

🔥 Absolute savage
For many, your wish may just come true.

@ljharb
Copy link
Member

ljharb commented Sep 14, 2021

As I said, I'm happy to make for a better error message, but so far the only commentary here has been "but my non-package.json use case is valid!" and nobody has actually provided repro repo where it would be reasonable to expect the plugin to work, but it fails.

With such a repo, I'd be happy to fix the problem. Without one, the only improvement that's possible is to ensure a better error message (but i'd still need a repro repo to figure out the call stack that the error is originating from).

@g1thuser
Copy link

g1thuser commented Sep 23, 2021

@ljharb

eslint-plugin-import doesn't apply for anything except node,

This is definitely wrong. I'm using this plugin for my typescript projects, which aren't node projects and therefore don't require a package.json. Furthermore I'm using eslint also for the relevant gulpfile.jsm files in these projects for the build and even for their .eslintrc.js files.

Only because you don't see any reasonable use cases to use this plugin without a package.json, this doesn't mean that there aren't any. And yes, my non-package.json use case is valid and, of course, also the one from @GiladShoham .

But as you have asked for a repro repo, here it is.

The two eslint config files in this repo are crashing eslint with different stack traces. The included rules are the only rules from eslint-plugin-import, which I'm using, which cause a crash. All other rules from this plugin are fine for me.

@ljharb
Copy link
Member

ljharb commented Sep 23, 2021

@g1thuser what you're using it for doesn't really have any bearing on what it's intended for. gulp is a node tool, for the record, and it's a best practice for node tools to be npm-installed per-project via package.json.

Thanks for the repo; i'll take a look.

@g1thuser
Copy link

@ljharb I don't care about best practices, if they are bringing only disadvantages to me. I have some very good reasons to use a central node repository in my case and gulp is, of course, npm installed in this repository.

@oalders
Copy link

oalders commented Nov 17, 2021

First off, thanks for providing this library. :)

At $work, we use Code::TidyAll to lint and tidy files of various languages and file types. This library (for reasons) copies files over to a temporary directory before linting or fixing them. In our case this worked fine until eslint-plugin-import was upgraded. Code::TidyAll is basically language-agnostic and it works on a list of provided files. Our package.json was not being included in the list of files to copy to the temp dir, since it wasn't getting linted. So, yes, we have a package.json, but no, it wasn't anywhere remotely near the files which were being linted because of how the library that harnesses all of the various linters and tidiers works.

So, I just wanted to point out that this kind of use case exists. We will work around this, but it did seem surprising that this was the case.

@ljharb
Copy link
Member

ljharb commented Nov 17, 2021

Being language-agnostic is simply not something that can ever be relied upon from individual packages in an ecosystem. I'm sorry for the inconvenience, and I'm certainly happy to accept PRs that:

  • improve the error message
  • avoid a crash, where that makes sense for the rule

Beyond that, if a tool is relying on something not needing to be present that the ecosystem expects is always present without exception, the tool is guaranteed to break in unpredictable ways. Note as well that package.json is conceptually required by node itself to determine how to parse a .js file - whether as ESM or CJS - so any tool that wants to handle node/npm projects already needs to ensure a package.json is present to work properly in a generic case.

@oalders
Copy link

oalders commented Nov 17, 2021

Thanks, @ljharb. I appreciate your thoughts. From my perspective, having a clearer error message would have been really helpful in debugging this.

@ariesclark
Copy link

I'm experiencing this issue as well, I use pnpm for my package manager which allows me to use package.yaml as my file of choice. This rule breaks when this is the case.

@ljharb
Copy link
Member

ljharb commented Jan 17, 2022

@RubyBB even if it didn't crash, it wouldn't be useful, because literally nothing in the ecosystem besides pnpm (apparently) uses package.yaml.

@ariesclark
Copy link

@RubyBB even if it didn't crash, it wouldn't be useful, because literally nothing in the ecosystem besides pnpm (apparently) uses package.yaml.

I wrote a quick dirty solution for my problem.

scripts:
  lint:es: >
    node -e "console.log(JSON.stringify(require(\"yaml\").parse(require(\"fs\").readFileSync(\"./package.yaml\", \"utf8\"))))" > package.json &&
    pnpm exec eslint . --fix && rm package.json || rm package.json

@ljharb
Copy link
Member

ljharb commented Jan 17, 2022

That seems like a good thing to do all the time anyways, since the entire JS ecosystem, including node itself (which checks "type", "imports", "exports", and "name"), only works with package.json.

@GiladShoham
Copy link

@ljharb node checks package.json inside the specific node_module, which makes sense to validate.
But not at the root of the project.
You can npm install many things and then delete the package.json from the root and I think nothing in the ecosystem will break except this plugin.
Actually, I'm working without a package.json in the root for 6 years now in many different projects. some are really big and complex. and didn't face any issues except this.

Still, if you want this to be validated, it shouldn't crash the entire eslint process.
And the error should be more clear.
If there was a rule I could just turn off it was much better.
As this plugin is part of the airbnb lint rules, which make it very hard to be removed.

@ljharb
Copy link
Member

ljharb commented Apr 2, 2023

If you npm install and delete package.json then what you’ve broken is your ability to manage your project’s dependencies; I’m not concerned about this plugin in that scenario. See #2096 (comment); I’m happy to avoid the crash and provide a better error tho.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

8 participants