Skip to content

Conversation

@pmdartus
Copy link
Contributor

This PR adds support for ESLint 7 to this package:

  • Update dependencies to make them compatible with ESLint 7
  • Add build step in CI to check if package work on both ESLint 6 and ESLint 7

Fixes #38

@pmdartus pmdartus requested a review from ekashida November 16, 2020 11:10
keys:
- v1-dependencies-{{ checksum "yarn.lock" }}
- v1-dependencies-{{ checksum "yarn.lock" }}-<< parameters.eslint >>
- v1-dependencies-
Copy link
Member

Choose a reason for hiding this comment

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

What is this cache key used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We make CircleCI cache the NPM dependencies between builds so we don't have to redownload them.
Based on your other comments, I will rework the CI config.

"scripts": {
"format": "prettier --write \"**/*.{js,md}\"",
"format:check": "prettier --check \"**/*.{js,md}\"",
"lint": "eslint",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"lint": "eslint",
"lint": "eslint .",

Seems like eslint@6 just prints the help options when you don't specify a path:
https://app.circleci.com/pipelines/github/salesforce/eslint-config-lwc/75/workflows/10ee9089-33c8-4f5e-9336-975ba1157ead/jobs/75

But eslint@7 is fine without it:
https://app.circleci.com/pipelines/github/salesforce/eslint-config-lwc/75/workflows/10ee9089-33c8-4f5e-9336-975ba1157ead/jobs/76

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch @ekashida. I think we need to run yarn lint first and then install a different eslint version to run the tests.


- run: yarn format:check
- run: yarn lint
- run: yarn test
Copy link
Member

Choose a reason for hiding this comment

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

Just to confirm, from the compatibility perspective, we're more interested in the test results because yarn lint just lints the source in this repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right.

@pmdartus pmdartus requested a review from ekashida November 18, 2020 10:38
@pmdartus
Copy link
Contributor Author

I updated the workflow based on your recommendation. Let me know what you think =)

Copy link
Member

@ekashida ekashida left a comment

Choose a reason for hiding this comment

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

lgtm!

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.

ESLINT version conflict with salesforce-app package

3 participants