Skip to content

Fix eslint errors by selectively enabling vue parser #20113

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Fix eslint errors by selectively enabling vue parser #20113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 31 additions & 4 deletions .eslintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ reportUnusedDisableDirectives: true

ignorePatterns:
- /web_src/js/vendor
- /templates/base/head_script.tmpl
- /templates/repo/issue/view_content/pull.tmpl
Comment on lines +6 to +7
Copy link
Contributor

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.

Copy link
Contributor

@wxiaoguang wxiaoguang Jun 24, 2022 •
edited
Loading

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.

Copy link
Member Author

@silverwind silverwind Jun 24, 2022 •
edited
Loading

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.

vuejs/eslint-plugin-vue#260

TBH, the vue ecosystem seems like a housefire, I'm not really motivated to even fix these.

Copy link
Contributor

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.

Copy link
Member Author

@silverwind silverwind Jul 11, 2022 •
edited
Loading

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.


parserOptions:
sourceType: module
Expand All @@ -15,9 +17,6 @@ plugins:
- eslint-plugin-html
- eslint-plugin-jquery

extends:
- plugin:vue/recommended

env:
es2022: true
node: true
Expand All @@ -29,7 +28,9 @@ settings:
html/html-extensions: [".tmpl"]

overrides:
- files: ["web_src/**/*.js", "web_src/**/*.vue", "templates/**/*.tmpl"]
- files: ["web_src/**/*.vue"]
parser: vue-eslint-parser
- files: ["web_src/**/*.js", "web_src/**/*.vue", "templates/**/*.tmpl", "docs/**/*.js"]
env:
browser: true
node: false
Expand Down Expand Up @@ -502,11 +503,37 @@ rules:
use-isnan: [2]
valid-typeof: [2, {requireStringLiterals: true}]
vars-on-top: [0]
vue/attribute-hyphenation: [2]
vue/attributes-order: [0]
vue/component-definition-name-casing: [0]
vue/component-tags-order: [2]
vue/first-attribute-linebreak: [2]
vue/html-closing-bracket-newline: [2]
vue/html-closing-bracket-spacing: [0]
vue/html-end-tags: [2]
vue/html-indent: [2]
vue/html-quotes: [2]
vue/html-self-closing: [2]
vue/max-attributes-per-line: [0]
vue/multiline-html-element-content-newline: [2]
vue/mustache-interpolation-spacing: [2]
vue/no-lone-template: [2]
vue/no-multi-spaces: [2]
vue/no-multiple-slot-args: [2]
vue/no-side-effects-in-computed-properties: [2]
vue/no-spaces-around-equal-signs-in-attribute: [2]
vue/no-template-shadow: [2]
vue/no-v-html: [2]
vue/one-component-per-file: [0]
vue/order-in-components: [2]
vue/prop-name-casing: [2]
vue/require-default-prop: [2]
vue/require-prop-types: [2]
vue/singleline-html-element-content-newline: [2]
vue/this-in-template: [2]
vue/v-bind-style: [2]
vue/v-on-style: [2]
vue/v-slot-style: [2]
wrap-iife: [2, inside]
wrap-regex: [0]
yield-star-spacing: [2, after]
Expand Down
1 change: 0 additions & 1 deletion templates/base/head_script.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ If you are customizing Gitea, please do not change this file.
If you introduce mistakes in it, Gitea JavaScript code wouldn't run correctly.
*/}}
<script>
<!-- /* eslint-disable */ -->
window.addEventListener('error', function(e) {window._globalHandlerErrors=window._globalHandlerErrors||[]; window._globalHandlerErrors.push(e);});
window.config = {
appVer: '{{AppVer}}',
Expand Down
1 change: 0 additions & 1 deletion templates/repo/issue/view_content/pull.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,6 @@
{{end}}
<div class="ui divider"></div>
<script>
<!-- /* eslint-disable */ -->
(() => {
const defaultMergeTitle = {{.DefaultMergeMessage}};
const defaultSquashMergeTitle = {{.DefaultSquashMergeMessage}};
Expand Down
5 changes: 3 additions & 2 deletions web_src/js/components/PullRequestMergeForm.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@
-d '{"context": "test/context", "description": "description", "state": "${state}", "target_url": "http://localhost"}'
-->
<div>
<!-- eslint-disable -->
<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>
Copy link
Member Author

@silverwind silverwind Jun 23, 2022 •
edited
Loading

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.

Copy link
Contributor

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.

Copy link
Member Author

@silverwind silverwind Jun 24, 2022 •
edited
Loading

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.

Copy link
Contributor

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


<div class="ui form" v-if="showActionForm">
<form :action="mergeForm.baseLink+'/merge'" method="post">
Expand Down