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

Add new JS linter rules #17699

Merged
merged 10 commits into from Nov 22, 2021
Merged

Add new JS linter rules #17699

merged 10 commits into from Nov 22, 2021

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Nov 17, 2021

Adds a few useful rules from eslint-plugin-github. Notable changes:

  • Forbid dataset usage, its camel-casing behaviour makes it hard to grep for attributes.
  • Forbid .then() and .catch(), we should generally prefer await for new code. For rare cases where they are useful, a eslint-disable-line directive can be set.
  • Forbid array#forEach, for-of is superior because it allows flow-control statements.
  • Add docs js to linting

Adds a few useful rules from eslint-plugin-github. Notable changes:

- Forbid dataset usage, its camel-casing behaviour makes it hard to
  grep for attributes.
- Forbid .then() and .catch(), we should generally prefer await for new
  code. For rare cases where they are useful, a eslint-disable-line
  directive can be set.
- Add docs js to linting
@silverwind silverwind added type/refactoring Existing code has been cleaned up. There should be no new functionality. topic/ui Change the appearance of the Gitea UI labels Nov 17, 2021
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Nov 17, 2021
.eslintrc Show resolved Hide resolved
@wxiaoguang
Copy link
Contributor

About "Forbid dataset usage", what's the recommended way to bind a variable to a DOM node?

For example, in the legacy code, we bind a SimpleMDE object to the textarea DOM node's dataset.

@silverwind
Copy link
Member Author

silverwind commented Nov 18, 2021

what's the recommended way to bind a variable to a DOM node?

I would recommend against such techiques but I think either jQuery's .data() (which is not really dataset when writing data iirc), or just plainly modifying the DOM node, e.g. el.mydata = {} may work.

@wxiaoguang
Copy link
Contributor

what's the recommended way to bind a variable to a DOM node?

I would recommend against such techiques but I think either jQuery's .data() (which is not really dataset when writing data iirc), or just plainly modifying the DOM node, e.g. el.mydata = {} may work.

I think we had better make an agreement and write it into the guideline.

  1. For jQuery, $.data() is fine.
  2. For plainly modifying the DOM node, I don't think it is a good idea. It will make potential conflicts and more mis-usages than dataset. Maybe we can introduce something like getNodeData()/setNodeData() in our framework, get/setNodeData can use dataset internally and skip eslint.

@silverwind
Copy link
Member Author

silverwind commented Nov 18, 2021

getNodeData()/setNodeData()

Sorry, but this seems overkill to me. dataset is just a wrapper around data-* attributes which have the big drawback that they can only store strings, so I would generally only use them to communicate data from server to client like we currently do. For data-* access use getAttribute, setAttribute, hasAttribute. I would consider disallowing .data() in favor of .attr() too.

Any other cases should aim to not attach non-string data directly to DOM elements to re-use later, this is a big anti-pattern. Stuff like that is not even possible with React/Vue for example, the canonical way is to use state variables because those outlive the lifetime of DOM nodes.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Nov 18, 2021

So the conclusion can be:

  1. For legacy code, $.data() can be used to bind some non-string data to elements, string data should use $.attr() or get/set/hasAttribute.
  2. For new code, we should never bind any data to DOM node, using node.dataset is also forbidden.

Right?

@silverwind
Copy link
Member Author

Yes, sounds right. I would also consider removing/refactoring the existing .data() cases in a future PR.

@wxiaoguang
Copy link
Contributor

Would you like to write the conclusion into the frontend guideline in this PR, or would we update the guideline in other PRs?

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Nov 18, 2021
@silverwind
Copy link
Member Author

I think we can handle it during review and/or via linter. I guess it shouldn't actually be hard to create a custom eslint rule that forbids .data() for example.

@wxiaoguang
Copy link
Contributor

Some conflicts with main branch

@zeripath
Copy link
Contributor

conflicts resolved.

@lunny
Copy link
Member

lunny commented Nov 22, 2021

make L-G-T-M work

@lunny lunny merged commit a159c31 into go-gitea:main Nov 22, 2021
wxiaoguang added a commit to wxiaoguang/gitea that referenced this pull request Nov 22, 2021
* the project board was broken, this PR fixes it, and refactor the code, and we prevent the uncategorized column from being dragged.
* improve the frontend guideline (as discussed in go-gitea#17699)
wxiaoguang added a commit that referenced this pull request Nov 22, 2021
* the project board was broken, this PR fixes it, and refactor the code, and we prevent the uncategorized column from being dragged.
* improve the frontend guideline (as discussed in #17699)
@KN4CK3R KN4CK3R mentioned this pull request Dec 2, 2021
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
* Add new JS linter rules

Adds a few useful rules from eslint-plugin-github. Notable changes:

- Forbid dataset usage, its camel-casing behaviour makes it hard to
  grep for attributes.
- Forbid .then() and .catch(), we should generally prefer await for new
  code. For rare cases where they are useful, a eslint-disable-line
  directive can be set.
- Add docs js to linting

* also enable github/array-foreach

* small tweak

Co-authored-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: techknowlogick <techknowlogick@gitea.io>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
* the project board was broken, this PR fixes it, and refactor the code, and we prevent the uncategorized column from being dragged.
* improve the frontend guideline (as discussed in go-gitea#17699)
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/code-linting topic/ui Change the appearance of the Gitea UI type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants