Can fastify exclude .eslintrc in the publish package?

[joshuaavalon]

Prerequisites

• I have written a descriptive issue title
• I have searched existing issues to ensure the issue has not already been raised

Issue

fastify includes .eslintrc in its publish package, which is the following:

{
  "extends": "standard"
}

This cause a error pop-up in VSCode everytime when I try to check the types of fastify.

ESLint: Failed to load config "standard" to extend from.

I do not use standard and do not have it installed. However, .eslintrc in the package has higher priority than the workspace ESLint config. So, when I try to inspect the TypeScript definition, ESLint will throw an error.

Can fastify exclude .eslintrc in the publish package? It does not cause any errors in real code but it is annoying.

I also think fastify should only include the necessary files in its package which contains build scripts, documentation and tests.

[mcollina]

This file is needed to successfully run the tests. However I would like to know more and confirm this with other users of vscode. Is this a problem for you too?

[joshuaavalon]

This file is needed to successfully run the tests. However I would like to know more and confirm this with other users of vscode. Is this a problem for you too?

I am not asking to remove it from the repository but remove it from the published package.

The files in package.json can define what actually includes in the package when you run npm publish.

It is common to only include necessary files in the publish package, for example, not including .github. Because it takes effect when you run npm publish, I don't believe it will change the tests.

[mcollina]

Can you provide steps to reproduce? We often need a reproducible example, e.g. some code that allows someone else to recreate your problem by just copying and pasting it. If it involves more than a couple of different file, create a new repository on GitHub and add a link to that.

[joshuaavalon]

@Fdawgs I have checked #4038 does not fix #4031.

I created sample repository.
Everytime you use Go To Type Definition, you can cause the error pop up.

#4038 does not fix it because the ESLint plugin check config from the directory of the file you are currently viewing.

I do not think there other solutions either using files in package.json or using a .npmignore to prevent .eslintrc include in the published package.

[mcollina]

@Eomm @Fdawgs @simoneb could any of you check this out? I'm not a VSCode user.
I think the fix in #4038 would do the trick.

[simoneb]

Personally I wouldn't see this as something strictly needing fixing, but I guess it's not a big deal to exclude the eslint configuration file from the published package, it shouldn't be necessary in the published package anyway.

@joshuaavalon can you send a PR for this? All you need to do is include .eslintrc in the .npmignore file.

[mcollina]

TBH, I don't think we should.

[simoneb]

I don't have a strong opinion either, but keeping it doesn't bring any value I suppose (standard won't be installed when you installed the package, since it's a dev dependency). So opening the source code will necessarily cause some form of error. Although there aren't strong reasons to keep it or remove it, I would vote for removing it since it's causing troubles to some people.

[mcollina]

Removing it would cause npm test to fail once all devDependencies are installed.

[simoneb]

Yes it would if you ran npm test in the fastify package's folder after you have npm installed it into another package. Is it something we want to cover? Because honestly I don't think I've ever done that myself

[mcollina]

Yes it would if you ran npm test in the fastify package's folder after you have npm installed it into another package. Is it something we want to cover? Because honestly I don't think I've ever done that myself

That's essentially the reason why we ship test and other artifacts. A user could reconstruct a working copy of fastify from the package.

[simoneb]

Interesting, I genuinely didn't know this was something we would intentionally cover. In fact, I pretty much thought that generally the opposite was true. Meaning, ship only the minimum necessary for production use, in order to keep the package as small as possible. Maybe this is especially a concern for frontend packages, but that's also the reason why sites like https://bundlephobia.com/ exist I guess.

[Fdawgs]

Interesting, I genuinely didn't know this was something we would intentionally cover. In fact, I pretty much thought that generally the opposite was true. Meaning, ship only the minimum necessary for production use, in order to keep the package as small as possible. Maybe this is especially a concern for frontend packages, but that's also the reason why sites like https://bundlephobia.com/ exist I guess.

Raised this last year: https://github.com/orgs/fastify/teams/plugins/discussions/5

[Uzlopak]

Tbh. I think you should not lint node_modules folder. So this is imho more an issue of the vscode extension than of fastify and other packages itself.

[joshuaavalon]

That's essentially the reason why we ship test and other artifacts. A user could reconstruct a working copy of fastify from the package.

I think shipping tests may be useful in some rare cases. However, is shipping linting configuration really necessary? People may want to run tests on the published code but I cannot think why people would like to lint on published package.

[simoneb]

I think shipping tests may be useful in some rare cases. However, is shipping linting configuration really necessary? People may want to run tests on the published code but I cannot think why people would like to lint on published package.

I would agree, but npm test in this repo runs linting as well, I guess that's the main reason

[Uzlopak]

I can not reproduce the issue.

[Uzlopak]

@simoneb
i guess this is related to fastify/workflows#9