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

Fix eslint errors by selectively enabling vue parser #20113

Closed
wants to merge 1 commit into from

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Jun 23, 2022

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.

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
@silverwind silverwind added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Jun 23, 2022
<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

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

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

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 23, 2022
Comment on lines +6 to +7
- /templates/base/head_script.tmpl
- /templates/repo/issue/view_content/pull.tmpl
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

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

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

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.

@Gusted Gusted added this to the 1.18.0 milestone Jun 24, 2022
@silverwind
Copy link
Member Author

Replacement PR: #20323

@silverwind silverwind closed this Jul 11, 2022
@silverwind silverwind deleted the fix-eslint branch July 11, 2022 21:59
@lunny lunny removed this from the 1.18.0 milestone Dec 20, 2022
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants