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

Loading too many package.json files #1243

Closed
glensc opened this issue Dec 16, 2022 · 14 comments · Fixed by #1281
Closed

Loading too many package.json files #1243

glensc opened this issue Dec 16, 2022 · 14 comments · Fixed by #1281

Comments

@glensc
Copy link

glensc commented Dec 16, 2022

Description

lint-staged attempts to load foreign package.json files, i.e other than /package.json in project root

Longer explanation:

I made a test in my workspace which resulted in demo-project/node_modules directory, which was not under .gitignore.

Having this done, I started to notice json errors when committing:

SyntaxError: Unexpected end of JSON input
    at JSON.parse (<anonymous>)
    at jsonParse (file:///app/node_modules/lint-staged/lib/loadConfig.js:30:43)
    at Object.load (/app/node_modules/lilconfig/dist/index.js:144:35)
    at async loadConfig (file:///app/node_modules/lint-staged/lib/loadConfig.js:65:20)
    at async Promise.all (index 60)
    at async searchConfigs (file:///app/node_modules/lint-staged/lib/searchConfigs.js:84:3)
    at async runAll (file:///app/node_modules/lint-staged/lib/runAll.js:128:24)
    at async lintStaged (file:///app/node_modules/lint-staged/lib/index.js:107:17)

which made me enable debug to see where it comes from:

--- /app/node_modules/lilconfig/dist/index.js~    2022-12-16 16:52:54.000000000 +0200
+++ /app/node_modules/lilconfig/dist/index.js    2022-12-16 16:55:47.000000000 +0200
@@ -141,7 +141,9 @@
             validateLoader(loader, loaderKey);
             const content = String(await fsReadFileAsync(absPath));
             if (base === 'package.json') {
+                console.log('load: ', absPath);
                 const pkg = await loader(absPath, content);
+                console.log('loaded: ', absPath);
                 return transform({
                     config: getPackageProp(packageProp, pkg),
                     filepath: absPath,
(END)

which printed and that it loaded tons of package.json from demo-project/node_modules.

I think it's bug that lilconfig is called in a way that it goes searches the files recursively:

I do not read from the readme it should do that, it should find the files from project root only.

Steps to reproduce

  1. open project where you have lint-staged configured
  2. mkdir test
  3. cd test
  4. yarn add typescript # or whatever
  5. cd ..
  6. edit node_modules/lilconfig/dist/index.js as above
  7. touch test
  8. git add test
  9. yarn lint-staged

Debug Logs

expand to view
COPY THE DEBUG LOGS HERE

Environment

  • OS: irrelevant
  • Node.js: irrelevant
  • lint-staged: 13.1.0
@glensc
Copy link
Author

glensc commented Dec 16, 2022

even simper reproducer:

  1. mkdir 1243
  2. echo kalasaba > 1243/package.json
  3. git add 1243/package.json
  4. yarn lint-staged

lint-staged must not try to load 1243/package.json file!

@glensc
Copy link
Author

glensc commented Dec 16, 2022

Workaround: add explicitly tell which config to use: lint-staged -c .lintstagedrc.json

@iiroj
Copy link
Member

iiroj commented Dec 16, 2022

Are your additional package.json files not valid JSON? It is intentional behavior for monorepo support.

@iiroj
Copy link
Member

iiroj commented Dec 16, 2022

After reading your comment again, to prevent loading them from node_modules/ you should probably add it to your .gitignore.

@antonk52
Copy link
Contributor

Hi, lilconfig maintainer here. Lilconfig can only search recursively towards file system root. This makes me think that it is not lilconfig that "discovers" newly added package json files in a new workspace node_modules. Looking at the lint staged source code it looks like it uses git to infer all not gitignored filepaths to look up configuration files.

https://github.com/okonet/lint-staged/blob/50f95b3d51e69074ab5ff5ddb7147828fcd85b7b/lib/searchConfigs.js#L62

Which makes sense for a mono repo support. All node_modules in a repository should be added to a gitignore file. +1 to @iiroj comment above.

@iiroj
Copy link
Member

iiroj commented Dec 16, 2022

Yes, exactly. We enumerate all possible files from git, and then pass those to lilconfig. 👍

@privatenumber
Copy link

Could there be a way to turn off config search? Or at least fail silently on JSON.parse?

I want to add lint-staged to Bun's codebase, but it errors trying to parse this empty package.json file used in a test fixture.

@iiroj
Copy link
Member

iiroj commented Apr 7, 2023

Would it make sense to just silently ignore all invalid package.json files? Lint-staged is only interested in a package.json file that contains the lintStaged key anyway...

@iiroj
Copy link
Member

iiroj commented Apr 7, 2023

@privatenumber I opened a PR here which would hide the JSON.parse error for invalid package.json files: #1281

Note that this doesn't change any exit codes because currently even with the error lint-staged basically just ignores the config file.

@privatenumber
Copy link

That works, thanks @iiroj 🙏

@iiroj
Copy link
Member

iiroj commented Apr 7, 2023

Hello @privatenumber and @glensc, would version 13.2.1 work any better?

@glensc
Copy link
Author

glensc commented Apr 11, 2023

@iiroj here's my reproducer:

if it passes, it's fixed!

@privatenumber
Copy link

Worked great for me, thanks @iiroj

@iiroj
Copy link
Member

iiroj commented Apr 11, 2023

@glensc to be precise, lint-staged does try to load the file so that it can autodiscover configuration for monorepo support. With the fix in 13.2.1 it just silently ignores errors from package.json files so the output should look a bit nicer.

I will mark this issue as resolved, thanks!

@iiroj iiroj closed this as completed Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

4 participants