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

Suppressing remaining lint warnings. #47

Merged
merged 5 commits into from
Aug 11, 2023

Conversation

sfdctaka
Copy link
Contributor

@sfdctaka sfdctaka commented Jul 6, 2023

Lint warnings for some html files will be left untouched. This is because Komaci's static analysis is too aggressive in enforcing its rule. The code is still valid even with the warnings. We need Komaci to be more flexible instead.

Interesting note. Eslint plugin cannot suppress lint warnings in html using //eslint-disable variants. It may be a feature which we may need to look into in eslint-plugin-lwc-graph-analyzer.

@sfdctaka sfdctaka requested a review from a team as a code owner July 6, 2023 20:39
Comment on lines 18 to 22
@api
titleValue = "";

@track
@api
descriptionValue = "";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless @api was used, in html, I was seeing this lint warning.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

I've not seen this warning, locally. It feels erroneous, given that there is no "child" component being implemented in this use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you update to v0.7.1 of eslint graph analyzer?

@@ -55,6 +56,7 @@ export default class FileUpload extends LightningElement {
this.descriptionValue = "";
this.errorMessage = "";
}
/* eslint-enable @lwc/lwc/no-api-reassignments */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because @api is now used new lint warning was shown. It seems as though the rules @salesforce/lwc-graph-analyzer/no-composition-unanalyzable-property-non-public and @lwc/lwc/no-api-reassignments work so well together.

For now I suppressed the warnings between 38-59.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

@@ -15,26 +15,27 @@ export default class FileUpload extends LightningElement {
@track
uploadingFile = false;

@track
@api
Copy link
Contributor

Choose a reason for hiding this comment

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

With the unnecessary warnings disabled at line 28 and line 30, I don't think these need to change away from @track anymore. And that in turn should alleviate the requirement for disabling no-api-reassignments on line 59.

@sfdctaka
Copy link
Contributor Author

sfdctaka commented Aug 8, 2023

@khawkins FYI, verified that removing template from errorPanel and making it a LWC bundle works.

@@ -11,6 +10,7 @@ export default class ErrorPanel extends LightningElement {

viewDetails = false;

@api
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't bother to make this @api. From Komaci's standpoint, it's not analyzable anyway, and it's not otherwise necessary to be exposed to components using this one in their composition.

@sfdctaka sfdctaka merged commit bafa250 into salesforce:main Aug 11, 2023
8 checks passed
@sfdctaka sfdctaka deleted the fileUploadLint branch August 11, 2023 16:42
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.

2 participants