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

Setup eslint plugin scaffold and basic CI actions #1

Merged
merged 16 commits into from
May 6, 2024

Conversation

haifeng-li-at-salesforce
Copy link
Contributor

  • Set up the scaffold for graphiql eslint plugin dev following the pattern from lwc-graph-analyzer and 'eslint-plugin-lwc`
  • Introduce a simple Rule enforce-fool bar from ESlint.org to verify the coding/test workflow working
  • Introduce lint, prettier and test actions

CODEOWNERS Outdated Show resolved Hide resolved
@sfdctaka
Copy link
Contributor

sfdctaka commented May 1, 2024

@haifeng-li-at-salesforce There are conflicts in this branch that you need to resolve.

jest.config.js Outdated Show resolved Hide resolved
.eslintrc Outdated Show resolved Hide resolved
.editorconfig Outdated Show resolved Hide resolved
.gitattributes Outdated Show resolved Hide resolved
.yarnrc.yml Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@khawkins
Copy link
Collaborator

khawkins commented May 1, 2024

I see both package-lock.json and yarn.lock. We should standardize on NPM or Yarn. I'd prefer NPM, as it doesn't require a separate install.

I've come to be of a mind to only use Yarn, pnpm, etc., if there's a specific set of features within those package managers that we're looking to adopt. Otherwise, vanilla NPM is the least effort to support.

@khawkins
Copy link
Collaborator

khawkins commented May 1, 2024

Also, with regards to using NPM: package-lock.json needs to be regenerated without nexus-proxy.repo.local.sfdc.net as the configured registry. Package lock entries should point to https://registry.npmjs.org.

package.json Outdated Show resolved Hide resolved
Copy link
Collaborator

@khawkins khawkins left a comment

Choose a reason for hiding this comment

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

I think those are my main comments for the initial review!

@sfdctaka
Copy link
Contributor

sfdctaka commented May 3, 2024

Can you check why prettier is failing on CI?

Copy link
Contributor

@sfdctaka sfdctaka left a comment

Choose a reason for hiding this comment

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

LGTM!

.eslintrc Outdated Show resolved Hide resolved
@haifeng-li-at-salesforce
Copy link
Contributor Author

Copy link
Contributor

@ben-zhang-at-salesforce ben-zhang-at-salesforce left a comment

Choose a reason for hiding this comment

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

loving it :)

@haifeng-li-at-salesforce
Copy link
Contributor Author

@khawkins
Copy link
Collaborator

khawkins commented May 6, 2024

@khawkins The below iscode snippet from komachi plugin: https://github.com/salesforce/eslint-plugin-lwc-graph-analyzer/blob/3abcba3c290a48f7067bebef39436f731487ac94/.eslintrc#L21

Reading about all of that configuration more, I guess it doesn't matter too much: the .eslintrc configuration in this case is only going to apply to to any linting we do ourselves within the project. And then we override the parser configuration anyway, the only place we run the plugin within the project.

So I guess the configuration is meaningless in our case. 🙂 I don't feel strongly about if/when we update that in the eslint-plugin-lwc-graph-analyzer project. I'll add a small story for it.

@khawkins
Copy link
Collaborator

khawkins commented May 6, 2024

@khawkins The below iscode snippet from komachi plugin: https://github.com/salesforce/eslint-plugin-lwc-graph-analyzer/blob/3abcba3c290a48f7067bebef39436f731487ac94/.eslintrc#L21

Reading about all of that configuration more, I guess it doesn't matter too much: the .eslintrc configuration in this case is only going to apply to to any linting we do ourselves within the project. And then we override the parser configuration anyway, the only place we run the plugin within the project.

So I guess the configuration is meaningless in our case. 🙂 I don't feel strongly about if/when we update that in the eslint-plugin-lwc-graph-analyzer project. I'll add a small story for it.

I guess it technically applies to the linting we apply to our project—not the test code. Sorry, lots of moving parts! So if we had code within our plugin that used ES6 constructs—which we don't seem to—then we'd want those parsing options, because otherwise ESLint analysis of our codebase would throw errors.

Anyway, since we don't have that problem, let's just leave the configuration out. And if and when we choose to update our codebase to leverage ES6 or TypeScript or whatever, we can revisit.

Copy link
Collaborator

@khawkins khawkins left a comment

Choose a reason for hiding this comment

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

This is g2g for me. 👍

@ben-zhang-at-salesforce ben-zhang-at-salesforce merged commit c6516ad into main May 6, 2024
17 checks passed
@ben-zhang-at-salesforce ben-zhang-at-salesforce deleted the scaffold branch May 6, 2024 22:14
@salesforce salesforce deleted a comment from github-actions bot Jun 10, 2024
@salesforce salesforce deleted a comment from github-actions bot Jun 10, 2024
@salesforce salesforce deleted a comment from github-actions bot Jun 10, 2024
@salesforce salesforce deleted a comment from github-actions bot Jun 10, 2024
@salesforce salesforce deleted a comment from github-actions bot Jun 10, 2024
@salesforce salesforce deleted a comment from github-actions bot Jun 18, 2024
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.

4 participants