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

feat: add eslint-plugin-jest to backend template #15050

Closed
wants to merge 1 commit into from

Conversation

alde
Copy link
Collaborator

@alde alde commented Dec 6, 2022

Hey, I just made a Pull Request!

Add eslint-plugin-jest to new backend plugins created by npx backstage-cli new.

When creating a new backstage plugin it prints an error as such:

    Error: Failed to load plugin 'jest' declared in '.eslintrc.js': Cannot find module 'eslint-plugin-jest'
    Require stack:
       - ...omitted.../plugin-backend/__placeholder__.js
       Referenced from: ...omitted.../plugin-backend/.eslintrc.js

    Warning: Failed to execute command yarn lint --fix

    🎉  Successfully created backend-plugin

This aims to fix that by including eslint-plugin-jest as a devDependency

Signed-off-by: Rickard Dybeck [email protected]

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)
  • All your commits have a Signed-off-by line in the message. (more info)

Add eslint-plugin-jest to new backend plugins created by `npx backstage-cli new`.

When creating a new backstage plugin it prints an error as such:

```
    Error: Failed to load plugin 'jest' declared in '.eslintrc.js': Cannot find module 'eslint-plugin-jest'
    Require stack:
       - ...omitted.../plugin-backend/__placeholder__.js
       Referenced from: ...omitted.../plugin-backend/.eslintrc.js

    Warning: Failed to execute command yarn lint --fix

    🎉  Successfully created backend-plugin
```

This aims to fix that by including eslint-plugin-jest as a devDependency

Signed-off-by: Rickard Dybeck <[email protected]>
@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2022

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage/cli packages/cli minor v0.21.2-next.2

@@ -40,6 +40,7 @@
"devDependencies": {
"@backstage/cli": "{{versionQuery '@backstage/cli'}}",
"@types/supertest": "{{versionQuery '@types/supertest' '2.0.12'}}",
"eslint-plugin-jest": "{{versionQuery 'eslint-plugin-jest' '27.1.6'}}",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already a dependency of the CLI, so shouldn't be needed

"eslint-plugin-jest": "^27.0.0",

What might be going on here is that the node_modules layout ends up in in such a shape that the ESLint plugin isn't reachable from the plugin itself. The workaround should be to make sure that the ESLint config bundled with the CLI resolves the plugins relative to the config itself, which means inserting require.resolve over here:

'plugin:jest/recommended',
. I'm not entirely sure what we'd be resolving though tbh, it would need to mimic the way that ESLint looks up these presets. Either way, it'd look something similar to what we do for example in the Jest config:
return { testEnvironment: require.resolve('jest-environment-jsdom') };

Do you want to dig into figuring out how to get that set up instead? otherwise I could get an issue going

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can take a look, but my nodejs tooling knowledge is not as good as it is for other languages

Copy link
Collaborator Author

@alde alde Dec 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would seem like adding this would also work - but it's dependent on filesystem path

❯ git --no-pager diff       
diff --git plugins/plugin-backen/.eslintrc.js plugins/plugin-backend/.eslintrc.js
index e2a53a6..a9a1fbe 100644
--- plugins/plugin-backend/.eslintrc.js
+++ plugins/plugin-backend/.eslintrc.js
@@ -1 +1,3 @@
-module.exports = require('@backstage/cli/config/eslint-factory')(__dirname);
+module.exports = require('@backstage/cli/config/eslint-factory')(__dirname, {
+  extends: ['../../.eslintrc.js'],
+});

edit: never mind, doesn't seem to work

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had another look at this and it turns out ESLint doesn't support my suggested solution of resolving absolute paths, or more like actively refuses it. Right now the proposed solution is to use peer dependencies for plugins and then tools to automatically install peer dependencies, I really don't want to do that because that'll mess with other deps 😅. A possible solution could be to separate out the ESLint config into its own package as that might flatten the dependency graph in some useful ways, but not sure.

Either way, it turns out that the people over at ESLint have been working on a new an improved config format that as far as I can tell will solve this issue and a lot of the other ones that we're seeing when it comes to configuring ESLint for a monorepo. The RFC is over at #15050, and implementation is underway in eslint/eslint#13481. Any changes we do to the way our ESLint config is packages and set up should be taking this new config format into account.

@jhaals jhaals assigned Rugvip and unassigned jhaals Dec 8, 2022
@github-actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

@github-actions github-actions bot added the stale label Dec 15, 2022
@github-actions github-actions bot closed this Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants