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

fix: use config directory as cwd, when multiple configs present #1091

Merged
merged 2 commits into from Feb 1, 2022

Conversation

iiroj
Copy link
Member

@iiroj iiroj commented Jan 19, 2022

This PR makes tasks run in the directory of the config file, when there are multiple configuration files.

In the case of only a single configuration file, tasks will still run in the current working directory, so this shouldn't really be a breaking change.

I also added some debug logging to getConfigGroups.

What do you think, @okonet?

@iiroj
Copy link
Member Author

iiroj commented Jan 19, 2022

I guess the only question is that should there be an info message about this...

@iiroj iiroj linked an issue Jan 19, 2022 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Jan 19, 2022

Codecov Report

Merging #1091 (9cdbd3e) into master (3a897ff) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1091   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           23        23           
  Lines          647       653    +6     
  Branches       169       173    +4     
=========================================
+ Hits           647       653    +6     
Impacted Files Coverage Δ
lib/runAll.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a897ff...9cdbd3e. Read the comment docs.

@iiroj
Copy link
Member Author

iiroj commented Jan 19, 2022

Maybe something like:

i Lint-staged detected multiple configuration files.
  Tasks will run independently from each directory.

@iiroj iiroj changed the base branch from master to cwd-flag January 23, 2022 07:03
@iiroj
Copy link
Member Author

iiroj commented Jan 23, 2022

I rebased this to #1094

Base automatically changed from cwd-flag to master January 23, 2022 11:16
@iiroj iiroj force-pushed the multi-config-cwd branch 2 times, most recently from eebd2da to fc46e54 Compare January 23, 2022 14:40
lib/runAll.js Show resolved Hide resolved
@iiroj iiroj requested a review from okonet January 24, 2022 15:58
@rbayliss
Copy link

Just wanted to note that I stumbled across this issue after realizing that my task binaries weren't being resolved as expected for subdirectory configurations. In our case, we've got a root configuration that covers several directories, then a subdirectory configuration that covers that one specific subdirectory.

After this change, tasks from both the subdirectory and root directory are behaving as I'd expect.

@iiroj
Copy link
Member Author

iiroj commented Feb 1, 2022

What do you think @okonet? Should we merge this and then investigate on how to merge/run nested configurations?

@iiroj iiroj merged commit 9a14e92 into master Feb 1, 2022
@iiroj iiroj deleted the multi-config-cwd branch February 1, 2022 18:01
@github-actions
Copy link
Contributor

github-actions bot commented Feb 1, 2022

🎉 This PR is included in version 12.3.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@bbugh
Copy link

bbugh commented Feb 1, 2022

Seems like there's some bugs or surprising behavior.

The changes here seem require that there be a lint-staged config in the top level directory, even if it's not needed.

This does not work, though I expected it to:

/ 
/project1/.lintstagedrc.json
/project2/.lintstagedrc.json

This does work:

/ .lintstagedrc.json
/project1/.lintstagedrc.json
/project2/.lintstagedrc.json

Unfortunately, the base .lintstagedrc.json cannot be an empty object or lint-staged exits with an error code.

Doesn't work:

{}

I tried putting in junk:

{
        "alsdifjasd": "echo"
}

but that resulted in this, even with the subdirectory files:

→ No staged files match any configured task.

I tried this:

{
        "*": ""
}

but got

The "file" argument must be of type string. Received undefined

Finally I just tried to print

{
	"*": "printf '%s\\n'"
}

and that worked as expected, checking and running the subdirectory configurations.

I'm not sure what the expectation is for how this should behave, but this was very surprising. Maybe it will help someone else in the future.

@iiroj
Copy link
Member Author

iiroj commented Feb 1, 2022

@bbugh thanks for the report. I guess for now there should be at least a single config file found for staged files, but maybe that shouldn't be the case?

We already have the case "no staged files matched any of the globs", so if no files match a config, that'd be the same thing.

@minhloi
Copy link

minhloi commented Feb 15, 2022

@iiroj I have the same issue as @bbugh did. In my project, each package has its own config file. But if all the staged files are in a single package, it will not work.

I think the reason is if it only uses a single config file, hasMultipleConfigs is false (although there are multiple configs)

https://github.com/okonet/lint-staged/blob/9cdbd3e93b36def73ccc00d516b9d767a1f24445/lib/runAll.js#L150

@iiroj
Copy link
Member Author

iiroj commented Feb 15, 2022

@minhloi yes, the logic for discovering configs by only checking from the staged files is wrong. We need to use git ls-files or similar instead.

@iiroj
Copy link
Member Author

iiroj commented Feb 15, 2022

@minhloi this will help: #1106

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

Successfully merging this pull request may close these issues.

Task working directoy when multiple configs
6 participants