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

Consider allowing running of programatic tasks via Node API #1398

Open
pastelsky opened this issue Feb 27, 2024 · 5 comments
Open

Consider allowing running of programatic tasks via Node API #1398

pastelsky opened this issue Feb 27, 2024 · 5 comments

Comments

@pastelsky
Copy link

pastelsky commented Feb 27, 2024

Description

Currently, the signature of a task in lint staged is —

(filenames: string[]) => string | string[] | Promise<string | string[]>

where it always expects a shell command as output that it runs via execa. However, consider adding support for promise returning functions instead. E.g.

import { doFoo } from 'package-foo';

export default {
   '**/*.js': async (filenames) => {
      console.log('Start a task')
      await doFoo(filenames);
      console.log('End a task)
  }
}

Advantages —

  1. In a few cases avoid shell execution cost all together (this can be in seconds in large projects using yarn)
  2. Allow for more flexibility in what's executed inside the task fn — including logging / telemetry etc

Given lint-staged is used in steps like pre-commit that need to be lightning quick, saving on shell invocation / yarn invocations costs can be significant. I understand there is value in the declarative nature of the current config — but optimizing for latency in ms is an important goal for something that runs in the critical path of dev workflow.

If this is allowed, the signature would become —

(filenames: string[]) => string | string[] | Promise<string | string[]> | Promise<void>

Where Promise<void> can indicate a freeform task.

Is this something that sounds interesting to you? Do you see any challenges with this?

@okonet
Copy link
Collaborator

okonet commented Feb 27, 2024

I really like it! Do you want to create a PR for that? @iiroj wdyt?

@iiroj
Copy link
Member

iiroj commented Feb 27, 2024

Is the functionality essentially the same as creating a separate Node.js script file, and having that be the linter? What benefit is there for defining it directly in the config?

It sounds doable, but is a separate pipeline to the existing thing being based on spawning processes with execa.

@iiroj
Copy link
Member

iiroj commented Feb 27, 2024

To answer myself: one benefit is that file names would be passed directly so there's less need for escaping etc.

@iiroj
Copy link
Member

iiroj commented Feb 27, 2024

This might be a good use of node:worker_threads so that we can run them in parallel and capture console output similar to execa.

@iiroj
Copy link
Member

iiroj commented Mar 5, 2024

Since we execute the current "function config" functions before running actual tasks and also validate the output to be string | string[], the only way to handle this in a backwards-compatible way would be to return more functions:

// lint-staged.config.js
export default {
  "*.js": (stagedFiles) => {
    return () => lintJsFiles(stagedFiles)
  }
}

the syntax of the returned functions could be:

type TaskFunction = (stagedMatchedFiles: string[]) => Promise<void>

but since they're returned from inside a config function, one could of course handle the matched filenames in there as well.

what do you think?

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

No branches or pull requests

3 participants