-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Fix eslint errors by selectively enabling vue parser #20113
Conversation
eslint currently gives some errors [1] because the vue parser when loaded via recommended config parses all files but can not handle hashbangs and newer ECMAScript features like the default eslint parser can. Selectively enable the vue parser only for .vue files and define rules explicitely to fix that issue. [1] https://drone.gitea.io/go-gitea/gitea/56685/1/4
<div v-if="mergeForm.hasPendingPullRequestMerge" v-html="mergeForm.hasPendingPullRequestMergeTip" class="ui info message"></div> | ||
<div v-if="mergeForm.hasPendingPullRequestMerge" class="ui info message"> | ||
{{ mergeForm.hasPendingPullRequestMergeTip }} | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Template code indicates this is just a string and not HTML, so v-html
(which above eslint-disable was targeting) was completely unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it isn't a plain string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is it? Template code maps it to a translation entry:
{{$hasPendingPullRequestMergeTip = $.i18n.Tr "repo.pulls.auto_merge_has_pending_schedule" .PendingPullRequestMerge.Doer.Name $createdPRMergeStr}}
pulls.auto_merge_has_pending_schedule = %[1]s scheduled this pull request to auto merge when all checks succeed %[2]s.
There's not HTML there so JS should see string. In any case, the warning is right, this is pretty unnecessary innerHTML
usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. It is HTML. createdPRMergeStr
- /templates/base/head_script.tmpl | ||
- /templates/repo/issue/view_content/pull.tmpl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would against to do these ignores.
Instead, please use eslint-disable
accurately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some eslint-disable
should be able to be written clearly in templates, but the author doesn't respond yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't able to get eslint-disable
to properly work in vue html files. Might have something todo about the plugin setup, I'm not sure.
TBH, the vue ecosystem seems like a housefire, I'm not really motivated to even fix these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If housefire
means something bad (I am not a native speaker so not sure I understand it correctly), most ecosystems also seem like housefire (eg, the problem I mentioned in eslint-plugin-html
problem ...)
If some problems are unfixable, maybe it's worth to vote to choose a better framework and start rewriting existing code, instead of being blocked forever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it seems the reason these existing <!-- /* eslint-disable */ -->
comments work is because of the vue parser that's loaded via plugin:vue/recommended
. As soon as I disables the vue parser, the HTML file fails to parse.
On the other hand, the vue parser can not deal with hashbang in the node scripts, only eslint's own parser can.
Replacement PR: #20323 |
eslint currently gives some errors because the vue parser when loaded via recommended config parses all files but can not handle hashbangs and newer ECMAScript features like the default eslint parser can (it did downgraded our parser from es2022 to es2020).
Selectively enable the vue parser only for .vue files and define rules explicitely to fix that issue.