Using the coverage reporter requires "@hapi/eslint-plugin" to be installed, even if it’s not used. #1017
Comments
|
|
|
My bad. That was probably because I |
|
Sure, no problem. 👍 |
Nargonath
added
non issue
|
This should probably be considered a bug. Lab should not require that Lab needs to either declare a peer dependency on |
|
I'm not sure we should declare it as a peer dep because it's dependent on it for the linter and coverage analysis feature. IMO it should come with
I wonder if the problem comes from |
Nargonath
added
bug
|
It is an eslint / npm thing. When you eslint a project it has no idea that lab initiated it and can only meaningfully search the base Any eslint plugin that depends on other plugins need to specify them as peer dependencies for it to always resolve correctly. |
|
Thats true. And it might be OK to wrestle with esling if you actually tell lab to lint the project. But it’s weird that it also requires these modules if linting is disabled and you only want a coverage report. |
Hmm, you are not quite right. Lab uses eslint for parsing the AST when checking for coverage. This needs the correct parser options for the current project. These are defined inside the |
|
If we don't want to add it as a peer dependency (as recommended by eslint), we could probably require in the embedded Line 4 in c7c1180 However, this is cumbersome and still has a peer dependency issue: If |
|
I didn't look at it this way. With your explanations it makes sense to add it as a peer dependency, thanks @kanongil for raising the issue. I'll open a PR to fix it. |
|
I've run into this same issue with @hapi/lab@24.3.2 & eslint@8.0.1, node 14.17.0 & npm 7.24.0. The error seems related to both this and #1021, because I don't get the same error if I downgrade eslint to 7.x.x npm test command: relavent dependency tree: And the error we get (which exits with code 0 so isn't getting flagged as a fail by our CI server 😕 ) |
|
Related to the above: I've also found the issue doesn't occur if I remove eslint from dev dependencies completely. Confused by that because due to the de-duplication npm does I will still have eslint@8.0.1 at the root of my project's node_modules which is just the same outcome as if I had eslint@8.0.1 as one of my devDependencies... |
The issue hapijs/lab#1017 includes a comment from someone who solved it by removing eslint as a dependency. As we should be using standardjs anyway we're just trying removing eslint to see if that moves us further.
DEFRA/water-abstraction-team#4 As a team, we are currently maintaining 11 repos and that's not including documentation, support and deployment-related stuff! What we inherited was not consistent; different CI tools were used for the same purpose and different CI steps were implemented. An effort was made to get this under control with [water-abstraction-orchestration](https://github.com/DEFRA/water-abstraction-orchestration). But to get a handle on it in such a way as the current team can manage, we're simplifying still further. This change updates `.github/workflows/ci.yml` to use a template we're applying across all the repos, amended to the requirements of the project. In the future, we hope to return to the lessons **water-abstraction-orchestration** taught us, if only to remove the duplication across the projects. But for now, the template hopefully simplifies and makes consistent our build process. **Notes** - Add .nvmrc We use it locally and in the CI to determine the version of Node to use. This repo didn't have one. So, we've added it and set the version to match what WRLS use for node.js. - Apply ci.yml template This is very much based on https://github.com/DEFRA/sroc-charging-module-api/blob/main/.github/workflows/ci.yml There is now one process, which means you'll just see the one item listed in the checks on GitHub. The steps are ordered in an effort to fail fast and/or where needed, for example, DB migrations before running tests and SonarCloud analysis after code coverage data has been generated. Specifically for this project, it also moves us away from using travis-ci.org, which was the first CI provider we integrated many, _many_ years ago. Defra has since migrated all their repos to travis-ci.com, then finally GitHub actions. Clearly, this one has been overlooked. - Update .labrc This is based on https://github.com/DEFRA/sroc-charging-module-api/blob/main/.labrc.js. We use the config file to ensure we generate the coverage output in a format needed by SonarCloud. - Add SonarCloud properties file Previously, the CI specified the values SonarCloud needs in its steps plus there were some in the web UI. But there are a bunch of other things we can tell SonarCloud about which makes managing SonarCloud much easier. So, we use `sonar-project.properties` files to set what the SonarCloud analysis step needs and what's useful in its UI. - Update README with new badges Also displays the badges we want in a layout that is consistent with most open source repos. - Switch to standardjs We had a problem trying to get the project to build again. We found [this issue](hapijs/lab#1017) which includes a comment from someone who solved it by removing eslint as a dependency. As we should be using standardjs anyway we removed eslint and switched to standardjs as part of this change. Normally, we would have done it as part of separate PR. - Remove setting table owners in migration code This breaks doing stuff locally and in our CI pipeline. The create migrations appear to be generated directly from pgAdmin so we also removed other statements which are not needed. - Update to hapi lab and code dependencies Hapi lab was listed x2 as a dev dependency. Once, as the new @hapi/hapi but the second time under the old `hapi` namespace. Also, old `lab` and `Code` were listed as the test dependencies. The old Hapi looks like it was bringing in a version of catbox-memory that depended on `big-time` but that wasn't actually getting installed. We removed it but to be on the safe side we have also updated the versions of Lab and Code to use those under the `@hapi` namespace.
Support plan
Context
What are you trying to achieve or the steps to reproduce?
Run lab with the coverage reporter and disabled linting in a project without
@hapi/eslint-plugininstalled:What was the result you got?
You get the following error:
This wasn’t the case at least until 24.1.1.
What result did you expect?
Lab shouldn’t require the eslint plugin if linting isn’t enabled.
The text was updated successfully, but these errors were encountered: