np icon indicating copy to clipboard operation
np copied to clipboard

Lint before publishing?

Open mmkal opened this issue 3 years ago • 8 comments

Description

If a lint scripts exists in package.json, run it after bumping the version field, and before publishing.

Is the feature request related to a problem?

Kind of. It makes the expiring-todo-comments eslint rule really helpful. You can put TODOs in the code like this one, which basically say "don't forget to do XYZ before the next major version". Then the lint task will automatically fail if you do forget about it.

Obviously, it'll also catch various other problems that might not otherwise be known about.

Possible implementation

Not sure exactly. Ideally it'd be able to go into prerequisite-tasks.js, but that might not be workable if the version has to be bumped first.

Alternatives

Manually put "npm run lint" in the "prepack" script. Short of that, hope that developers have good memories.

mmkal avatar Dec 04 '20 00:12 mmkal

Umm, sounds like a completely unrelated feature to np. All we're aiming to do is make the publishing process of npm packages smoother.

Running the lint ideally should be part of your internal process. The problem with implementing something like this could be that people run lint --fix in the lint in some cases, and this could cause undesirable behaviours in some cases.

Also. bumping the version before running lint to make way for expiring-todo-comments rule means we will have to rollback the entire thing and also produce the complete error log in the case the lint errors.

For me, it feels like too many edge cases to be handled by np directly.

dopecodez avatar Dec 05 '20 08:12 dopecodez

My thought was it's about as related as ensuring the package is built and tested. The idea is that lint can be run internally/as part of CI it still wouldn't catch errors related to the version number.

Re making publish smoother. I didn't think that was the aim of this package - I thought it was to make publishing "better". It succeeds at better, it fails at smoother. It's "smoother" to just do npm publish because that doesn't do any pesky validation steps that might fail! np is safer and more helpful (and obviously overall smoother if what you actually want to do is check-clean-install-test-bump-publish-push-create-draft-release, which it usually is).

Re rollback - I guess I've never used np with a non-clean git status, but isn't that one of the validations it does? Wouldn't rollback just be git checkout . or similar?

mmkal avatar Dec 05 '20 17:12 mmkal

I don't think this is a good idea for np. It's making too many assumptions on how a lint run script would work. People might use it for interactive linting where it doesn't exit on its own, for example. I think it's better to just do what everyone already does and put linting in the test run script. If you don't want it in the test run script, you could also put it in any of the npm lifecycle hooks: https://github.com/sindresorhus/np#npm-hooks

sindresorhus avatar Dec 07 '20 05:12 sindresorhus

Fair enough. I didn't mean to suggest it should be a new default behaviour, necessarily, though. Maybe something configurable.

I don't think adding it to test quite covers this scenario, it would succeed since as far as I'm aware test runs before the version bump (and it would then publish the package despite the todo not being done, and put master in a broken state).

Also agreed re lifecycle hooks, I've solved this before by adding it to prepack. Not all libraries will do that though and it feels like a bit of a hack, so I suppose I'm curious how others have solved this problem.

mmkal avatar Dec 07 '20 11:12 mmkal

You could use the version hook: https://docs.npmjs.com/cli/v6/commands/npm-version

sindresorhus avatar Dec 08 '20 08:12 sindresorhus

Sounds like this could be solved with a recipe in the readme.

sindresorhus avatar Dec 08 '20 08:12 sindresorhus

most projects I've seen ensure that lint is run on a pre commit / push basis. Since np checks for unclean stage, it would be impossible to publish unlinted code.

victornpb avatar Apr 21 '22 01:04 victornpb

This can easily be done by adding "preX" where X stand for the npm's script name. Something like this:

{
  ...
  "scripts": {
    "prenp": "lint away!",
    "np": "np",
    ...
  }
  ...
}

Wouldn't you agree?

Asaf-S avatar Sep 04 '23 13:09 Asaf-S