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

Adds ESLint package @react-three/eslint-plugin #2698

Merged
merged 8 commits into from
Jan 12, 2023

Conversation

itsdouges
Copy link
Contributor

@itsdouges itsdouges commented Jan 7, 2023

Hello! This pull request adds a new ESLint plugin package with no new rules (a stub one exists, the actual new rules will be added in a subsequent PR). Putting this up early for discussion, I think there is value to be had from writing lint rules for R3f, such as code that can be statically analyzed for problematic patterns (e.g clones in frame loops!).

For specific tasks done in the pull request I've:

  • Created a new package @react-three/eslint-plugin
  • Created a script codegen to generate the rules index, configs, plugin index, and README.
  • Added a command to the root package json to generate the files yarn codegen:eslint
  • Adds two eslint configs (all + recommended)

Tasks before merge

  • Fix failing CI
  • Test that the rules and configs are working as expected
  • Align that we're happy adding this @CodyJasonBennett @drcmda
  • Come up with a description for the package
  • Create a changeset for the first release of the package

Subsequent tasks

  • Run codegen on commit if rules folder was changed
  • Complete stub rule

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 7, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit d67f238:

Sandbox Source
example Configuration

@@ -9,16 +9,8 @@ module.exports = {
'<rootDir>/packages/test-renderer/dist',
'<rootDir>/test-utils',
],
// coverageThreshold: {
Copy link
Contributor Author

@itsdouges itsdouges Jan 7, 2023

Choose a reason for hiding this comment

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

Removed this since it was commented out.

coverageDirectory: './coverage/',
collectCoverage: true,
collectCoverage: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turned this to false since it was on for all unit tests, annoying! It's still enabled for yarn test.

@@ -37,6 +37,7 @@
"validate": "preconstruct validate",
"release": "yarn build && yarn changeset publish",
"vers": "yarn changeset version",
"codegen:eslint": "cd packages/eslint-plugin && yarn codegen",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does yarn@1 have workspaces-run inbuilt? Need to search over docs.

await writeFile(filepath, code)
}

async function generate() {
Copy link
Contributor Author

@itsdouges itsdouges Jan 7, 2023

Choose a reason for hiding this comment

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

Surprised this doesn't exist as a standalone npm pkg! Let me know if it does and my searchfu needs work.

@@ -0,0 +1,5 @@
---
'@react-three/eslint-plugin': minor
Copy link
Contributor Author

@itsdouges itsdouges Jan 9, 2023

Choose a reason for hiding this comment

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

Figured we just keep bumping minors until we release a major version. This will release the package under v0.1.0.

@@ -0,0 +1,208 @@
import type { Rule } from 'eslint'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This module generates most content inside the eslint plugin, including: rule index, configs, and readme. It is a very simple implementation.


export default {
plugins: ['@react-three'],
rules: {},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No rules are recommended yet!

}

await generateRuleIndex(rules)
await generateConfig('all', rules)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we ever want to add another config it's just a matter of calling generate config again. Perhaps we could introduce a native config if that made sense?

@itsdouges
Copy link
Contributor Author

@drcmda do we wait for @CodyJasonBennett to review before merging?

@CodyJasonBennett
Copy link
Member

No, all good here. Would be good to follow up with rules based on our docs for context/loop related patterns.

@itsdouges
Copy link
Contributor Author

@CodyJasonBennett no problem, can you link to the docs or even better list them here/in an issue? Will make it easier to not miss anything.

For getting this merged can someone hit the merge button? Alternatively you can also give me access to merge 😉.

@CodyJasonBennett
Copy link
Member

I documented these in #2701. @drcmda can you give @itsdouges write & publishing access?

@itsdouges
Copy link
Contributor Author

Hey folks happy to make a start on some lint rules, would like to get this merged first to keep the changes scoped though. When you get a chance could we merge please?

@CodyJasonBennett CodyJasonBennett merged commit 1e0ca98 into pmndrs:master Jan 12, 2023
@itsdouges
Copy link
Contributor Author

Thanks Cody 🙏

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

3 participants