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

feature: add no-trigger-core-import #1175

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

jacobparis
Copy link
Collaborator

@jacobparis jacobparis commented Jun 21, 2024

  • Creates an ESLint plugin that disallows importing from @trigger.dev/core and @trigger.dev/core/v3 directly (favouring more specific exports) and autofixes all of them in the codebase
  • Adds fine grained modules to @trigger.dev/core so they can be happily imported from the webapp
  • Installs the prerequisite eslint plugins required to make this work on the webapp

This PR should be mergeable on its own, designed to minimize conflicts.

> ~/Projects/trigger.dev/apps/webapp
pnpm lint --fix

Running this command with the following lint config takes about 30 seconds on my machine and fixes all of the imports in the project (approximately 140 files). After this PR lands, you can autofix and push the corrected imports at your leisure. Anyone with work in progress should be able to pull this commit+plugin and then autofix their pending files before to avoid merge conflicts.

I did not commit this config into this PR as that would cause the linter to fail, which we don't want to happen until a followup PR that comes with all the fixes so the linting is green again.

  "plugins": [
    "@trigger.dev/eslint-plugin",
    "react-hooks",
    "@typescript-eslint/eslint-plugin",
    "import"
  ],
  "parser": "@typescript-eslint/parser",
  "overrides": [
    {
      "files": ["*.ts", "*.tsx"],
      "rules": {
        // Autofixes imports from "@trigger.dev/core" to fine grained modules
        "@trigger.dev/no-trigger-core-import": "error",
        // Normalize `import type {}` and `import { type }`
        "@typescript-eslint/consistent-type-imports": [
          "error",
          {
            // the "type" annotation can get tangled and cause syntax errors
            // during some autofixes, so easier to just turn it off
            "prefer": "no-type-imports"
          }
        ],
        // no-trigger-core-import splits imports into multiple lines
        // this one merges them back into a single line
        // if they still import from the same module
        "import/no-duplicates": ["warn", { "prefer-inline": true }],
        // lots of undeclared vars, enable this rule if you want to clean them up
        "turbo/no-undeclared-env-vars": "off"
      }
    }
  ],
  "ignorePatterns": ["seed.js"]
}

In order to make VS Code's eslint plugin work with the webapp properly, I also had to add the following to my .vscode/settings.json. I don't know if there's a better solution for folks working on more than one package.

{
  "eslint.workingDirectories": ["apps/webapp"]
}

✅ Checklist

  • I have followed every step in the contributing guide
  • The PR title follows the convention.
  • I ran and tested the code works

Testing

[Describe the steps you took to test this change]


Changelog

[Short description of what has changed]


Screenshots

[Screenshots]

💯

Copy link

changeset-bot bot commented Jun 21, 2024

⚠️ No Changeset found

Latest commit: fb94a4a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@ericallam ericallam force-pushed the eslint-no-trigger-core-import branch from 380794e to 7270f48 Compare June 27, 2024 13:37
@ericallam
Copy link
Member

@jacobparis I don't think it's a good idea to remove the type imports, I'd prefer if we used type imports instead of stripping them out. Is there a reason this was needed?

@jacobparis
Copy link
Collaborator Author

@jacobparis I don't think it's a good idea to remove the type imports, I'd prefer if we used type imports instead of stripping them out. Is there a reason this was needed?

It's not strictly needed but the eslint autofixing with types sometimes doubles them like import type { type ArticleSchema } which causes syntax errors that need to be manually fixed and I wanted to present a solution that didn't require manual intervention

A clean solution would be to run the autofix to remove all type imports and then change the lint rule to run again and add them all from a clean state, which should avoid it getting tangled

But I have no strong opinions here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants