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 LitElement to recommended config #118

Merged
merged 3 commits into from
Sep 18, 2023
Merged

feat: add LitElement to recommended config #118

merged 3 commits into from
Sep 18, 2023

Conversation

43081j
Copy link
Owner

@43081j 43081j commented Sep 16, 2023

Introducing the invalid-extends rule into the best practice config resulted in a larger breaking change than expected: LitElement repos fail to lint without introducing an elementBaseClasses setting.

Since a large chunk of our consumers are lit users, it makes sense to add it to the recommended config so it doesn't need configuring individually.

Introducing the `invalid-extends` rule into the best practice config
resulted in a larger breaking change than expected: LitElement repos
fail to lint without introducing an `elementBaseClasses` setting.

Since a large chunk of our consumers are lit users, it makes sense to
add it to the recommended config so it doesn't need configuring
individually.
@43081j
Copy link
Owner Author

43081j commented Sep 16, 2023

@keithamus what do you think about this? i'm torn

on one hand, it'd save people time since they can upgrade without many config changes.

on the other, it means error messages in non-lit repos will say things like should extend HTMLElement or LitElement

basically without this it means many consumers will have to introduce a settings object to specify LitElement as a base class

i suppose the ideal is a lit config, but im just trying to think of a way to make the migration a bit easier (from 1.x to 2.x)

Copy link
Collaborator

@keithamus keithamus 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 the PR is fine as is but I also wonder - could we use heuristics to determine if its a lit project, e.g. reading from package.json?

@43081j
Copy link
Owner Author

43081j commented Sep 17, 2023

i did wonder that too, maybe we could get away with just require.resolve('lit')? it isn't exact, in that it could be a deep dependency, but maybe that's good enough?

otherwise, we could look for a package.json and see if it contains lit (keep in mind eslint is sync, so we would be using readFileSync etc)

edit: i've updated this to use the require.resolve method, let me know if you think that'll be fine

Copy link
Collaborator

@keithamus keithamus 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 this is great!

@43081j 43081j merged commit 9f4d509 into master Sep 18, 2023
3 of 4 checks passed
@43081j 43081j deleted the default-lit branch September 18, 2023 12:19
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