Allow defining a template for how paths should be passed to the command

[okonet]

At this moment lint-staged will always pass file paths as arguemnts at the end of the string:

npm run-script command -- path/to/staged/file1 path/to/staged/file2
eslint -- path/to/staged/file1 path/to/staged/file2

In order to support exotic cases like #136 and #18 we might want to redefine this. One way of doing this would be to pass a "template" string to how paths should be interpolated. Something like this:

{
  "*.jpg": "${ path } | imagemin",
  "*.html": "gulp my-task -g ${ paths }",
  "*.js": "eslint" // This is the current way of doing "eslint ${ paths }"
}

lint-staged will try to interpolate those strings when running tasks.

1. The piping example will require calling filepath | imagemin for each path in paths. Not sure how to do that. Different type of token (path instead of paths?)
2. Better error handling: since templates may be broken, lint-staged should help users find the mistake

Another use cases: #404

I'm open for a discussion and suggestions on the API of this feature.

[mkhazov]

As I know, imagemin-cli requires output file or dir specified, otherwise it will write to stdout. So regarding 1, probably the token should be path to allow things like imagemin $path --out-dir=$(dirname "${path}")

[okonet]

Good suggestion! Here is how I'm using it now: https://github.com/reactvienna/brand/blob/master/package.json#L9 but I agree this can be more flexible. @mkhazov do you want to make a stub and create a PR?

[adam-moss]

As an additional feature alongside #214 I'd like to see some sort of option allowing the pre-processing of files to support triggering other things, notably unit tests, on a per-script basis.

For example if I had something like

{
    "*.js*": "nyc jasmine"
}

Then rather than pass *.js I'd want to be able to pass *.spec.js - i.e. the unit test associated with the file.

That way in a precommit action assurance can be given that *.js hasn't borked the test suite, and similarly using some of the config attributes of nyc the commit could be aborted if coverage wasn't at the appropriate level.

This would probably introduce a bit of complexity however, especially if *.js and *.spec.js were both staged, and would probably being a breaking change for sure:

{
    "lint-staged": [
        {
            "include": "*.js",
            "exclude": "*.spec.js",
            "pre-process": "something first",
            "process": {
                "command": "the thing",
                "params": [ "x", "y", "z" ]
            },
            "post-process": "something after",
            "any other flag": "and its value"
        }
    ]
}

[okonet]

@adam-moss I think pre- and post- processing is a bit out of scope of this package since the goal of it to make the setup as simple as possible.

I believe test runners should be smart enough to be able to figure out what tests needs to be run for a given file. I've implemented this for Jest for instance and it works great for my use cases.

[okonet]

Related #201

[luftywiranda13]

I agree with this:

I think pre- and post- processing is a bit out of scope of this package since the goal of it to make the setup as simple as possible.

please note that this is my personal thought 😄

this package's named lint-staged, and its description is run linters on git staged files. Although we can add support for pre or post processing by utilizing lint-staged but lint-staged wasn't made for and also not optimized for those kind of things. IMO we need to focus on what problem this package is trying to solve.

that's the reason why I created a separate project called remove-lockfiles, even though i could just tricked lint-staged with some hacky config to do the things i wanna do.

to me personally, for a pretty simple stuff, i'd add an npm script then utilize husky to run it as a part of git hooks

[okonet]

Another use case is to be able to support any library/linter without modyfing them: facebook/flow#4189

[sudo-suhas]

@okonet I had some rough ideas regarding this. What about a plugin system? We pass the plugin the command/bin, the relevant files and it should return the args which we would then pass to execa. There are some complications due to the chunked execution but it should be doable.

[okonet]

@sudo-suhas I actually like this pretty much. Ideally, we would remove the chunking logic to the plugin as well since it makes the core much more complex IMO. How the API would look like for users i.e. how would you plug a plugin into the config?

[sudo-suhas]

We could do something similar to eslint and allow for defining the module using a string(ex: plain for lintstg-plugin-plain). We could also allow users to configure the function using lint-staged.config.js without mandating a npm module for their requirement.

[okonet]

allow for defining the module using a string(ex: plain for lintstg-plugin-plain)

After having tons of issues with ESLint just because of that, I think I'll be against this but let's continue discussing it. I still don't understand how this will look like in the lint-staged config.

We could also allow users to configure the function using lint-staged.config.js

This is a more explicit way of doing this but will limit the user base significantly because of complexity. And since the goal of this feature is to solve problems with popular libraries, I'd prefer us to make it accessible as possible.

[okonet]

@sudo-suhas the more I think about it the less I think that plugins is a way to go since they will complicate not only the setup but the maintaining part of the lib since we will either need to convert to mono repo or support multiple repos for plugins. Since the scope is limited, I'd rather stick to a more simple solution for now.

[okonet]

Another important question if implicit syntax { "*.js": "eslint" } should be deprecated in favor of more explicit one i.e. {"*.js": "eslint <paths>" }

[heavybeard]

any update on this? It is very useful

[glennreyes]

Since I have a similar use case for this I'm proposing following:

{
  "*.{ts,tsx}": [
    ["tsc --noEmit -p tsconfig.json"],
    "tslint",
    "git add"
  ],
  "*.graphql": [
    ["apollo client:codegen types", { "skipArgs": true, "ignore": "**/__generated__" }],
    "git add"
  ]
}

This enables following:

• Similar to babel plugin configs, we can optionally pass commands as an array and pass a scoped config in the second argument that applies only to the given command. This way we can potentially get rid of the global config object (introduces a breaking change).
• We can then add options to skip adding the path as an argument to the command, eg. skipArgs or even add option to support templating, eg.

argsTemplate: paths => `--includes=${paths.join(',')}`;

Any thoughts?

[okonet]

That's a great proposition! Especially keeping in mind it allows being implemented incrementally without breaking changes. This would allow closing #277 and refactor #385 and #534 to be a local options. It will also will make possible #557

I have a few questions:

1. Would template function only be possible in the config defined in JS?
2. Do we need skipArgs if we'd have argsTemplate? Looks like they are mutually exclusive.
3. I'm not sure about the name argsTemplate. Instead, I'd propose to just name it args with the following signature: (files: File[]) => string[]. The string is what's going to be passed to the command as arguments. Default implementation would be something like files => files.join(', '). This would allow achieving skipping of arguments by defining it as args: [] and I assume it would even work in JSON / YML formats.

[okonet]

@glennreyes the only corner case I see that won't be covered by your proposition is the case with piping arguments. See the original issue. Do you think we could also tackle this case somehow?

[yuki-yano]

I have commented out the file name passed to scripts as workaround.

  "scripts": {
    "typecheck": "tsc --project . --noEmit #",
  },
  "lint-staged": {
    "*.{ts,tsx}": [
      "yarn run typecheck",
      "git add"
    ]
  }

[Guria]

I have commented out the file name passed to scripts as workaround.

Unfortunately, it wouldn't work on windows

[okonet]

Doesn’t anyone want to work on this one?

[iiroj]

Would something like supporting a [filename] parameter (like here #635) in the command work? additionally, we might include a passFilename: false option (by default true) for cases where the command would fail with filename as the argument (like the tsc above).

I can contribute this next week if you like, @okonet

[okonet]

After thinking a lot about it, I like the proposition by @glennreyes the most but it's not covering all the corner cases. Considering all mentioned use cases are rather advanced, I'd propose to only support those in the CJS module format (lintstaged.config.js) using functions.

My proposal

module.exports = {
  "*.js": [
    "eslint", // This is how lint-staged works now
    (paths: FilePath[]): string => {
      // In this case we won't do any magic and will expect users to define the command by themselves
      return `someCommand "${files.join(', ')}"`
    }
  ]
}

This would give the total flexibility without adding additional DSL and parameters to the library.

Thoughts?

@iiroj I also think this is also much easier to implement and test.

[iiroj]

@okonet Using functions would make it possible to also leave the globbing to the supplied function, thoughts?

[okonet]

Yeah I was thinking about how to solve all long standing issues related to globbing but I think that would make the config overly complicated. Don’t you think so? We would still need to keep the object since that’s what’s cosmiconfig expects. But we could provide a second argument with all staged files to the function for users who’d like to bail out from our globbing and do that themselves. We could
use named arguments, of course. Thoughts?

[iiroj]

Maybe the proposal above is simpler, since the filename array always matches the glob. It's logical that way.

[okonet]

Not sure what you mean. Can you add an example?

[okonet]

I just realized that I’m order to get all files one could use * as a glob pattern. Simple!

[iiroj]

Ah, sorry, I meant that maybe your proposal is more logical, because for **/*.js, the files array will always match that. And as you say, if one wants to implement their own logic, the * would work. Sounds good to me!

[iiroj]

@okonet I created the basis for this feature here: 7ca1f6c

EDIT: 👆 doesn’t pass test, but will start working on this more tomorrow.

Makes me wonder if it should be possible to return an array of strings, for example if the binary only allows a single filename input, that would each get executed seperately.

Thoughts?

[okonet]

for example if the binary only allows a single filename input, that would each get executed seperately

That's a great and quite useful use case! I was wondering the same thing an wasn't sure if my solution would cover that. How would array of strings cover that? Would it be the core that would determine the behavior?

[okonet]

The implementation is looking good to me. Can't wait for you PR! Could you also address #635 while you on it? I feel like they are related.

[okonet]

Closed in #639