-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Can fastify exclude .eslintrc in the publish package? #4031
Comments
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 It is common to only include necessary files in the publish package, for example, not including |
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. |
@Fdawgs I have checked #4038 does not fix #4031. I created sample repository. #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 |
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. |
TBH, I don't think we should. |
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. |
Removing it would cause |
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 |
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 |
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. |
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 |
I can not reproduce the issue. |
@simoneb |
Prerequisites
Issue
fastify includes
.eslintrc
in its publish package, which is the following:This cause a error pop-up in VSCode everytime when I try to check the types of fastify.
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.
The text was updated successfully, but these errors were encountered: