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

Refactor branch/tag selector to Vue SFC #23421

Merged
merged 8 commits into from
Mar 14, 2023

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Mar 11, 2023

Follow #23394

There were many bad smells in old code. This PR only moves the code into Vue SFC, doesn't touch the unrelated logic.

update: after 5f23218 , there should be no usage of the vue-rumtime-compiler anymore (hopefully), so I think this PR could close #19851

@wxiaoguang wxiaoguang force-pushed the refactor-branch-selector branch from 2e6814b to cbbab48 Compare March 11, 2023 11:53
@lunny lunny added topic/ui Change the appearance of the Gitea UI type/refactoring Existing code has been cleaned up. There should be no new functionality. labels Mar 11, 2023
@wxiaoguang wxiaoguang force-pushed the refactor-branch-selector branch 2 times, most recently from 53bf4c3 to 0e20b2a Compare March 11, 2023 12:30
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Mar 12, 2023
@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 Mar 13, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 13, 2023
web_src/js/svg.js Outdated Show resolved Hide resolved
Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

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

😒

@wxiaoguang
Copy link
Contributor Author

Thank you. The code is quite old and complex, I do not want to change too much 😂

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 13, 2023

More details of this component:

Old code has too many problems:

  • // TODO: fix this anti-pattern: side-effects-in-computed-properties, it's really fragile, it would cause problems sooner or later ...
  • The using of jQuery: $().val(), $.trigger(), etc. jQuery shouldn't be used in Vue ....

In the future, we could rewrite this component to make it clear, then we would have enough time to do a full test of it.

So, it's better to leave the code rewriting together at the time, then we do not need to spend twice time on full-test.

@codecov-commenter
Copy link

codecov-commenter commented Mar 14, 2023

Codecov Report

Merging #23421 (41f24bc) into main (d56bb74) will increase coverage by 47.18%.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff            @@
##           main   #23421       +/-   ##
=========================================
+ Coverage      0   47.18%   +47.18%     
=========================================
  Files         0     1153     +1153     
  Lines         0   152151   +152151     
=========================================
+ Hits          0    71798    +71798     
- Misses        0    71874    +71874     
- Partials      0     8479     +8479     

see 1153 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

# Conflicts:
#	web_src/js/components/RepoBranchTagDropdown.js
#	web_src/js/svg.js
@wxiaoguang wxiaoguang force-pushed the refactor-branch-selector branch from 7b6bb1c to 9f0fa2a Compare March 14, 2023 05:38
@wxiaoguang wxiaoguang force-pushed the refactor-branch-selector branch from 9f0fa2a to 5f23218 Compare March 14, 2023 05:41
@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 14, 2023

Some details about the changes after 2 LGTM:

  • 5f23218

    • fix a bug in code, the size should be integer type
    • generate svg by function, instead of template: avoid using vue runtime compiler, then Gitea works with disabled unsafe-eval CSP.
  • 3c698d2

    • declare vue config explicitly (the same as the default before), otherwise there will be a warning in console log

They don't affect the reviewed logic.

@lunny lunny merged commit ac8d71f into go-gitea:main Mar 14, 2023
@lunny lunny removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 14, 2023
@wxiaoguang wxiaoguang deleted the refactor-branch-selector branch March 14, 2023 09:51
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 15, 2023
* giteaofficial/main: (33 commits)
  Bump webpack from 5.75.0 to 5.76.0 (go-gitea#23484)
  Replace Less with CSS (go-gitea#23481)
  Fix 'View File' button in code search (go-gitea#23478)
  Use `gitea/test_env` image instead of `golang` (go-gitea#23455)
  Skip DB tests duplicate runs on push to branches (go-gitea#23476)
  Update app.example.ini (go-gitea#23480)
  [skip ci] Updated translations via Crowdin
  Fix due date being wrong on issue list (go-gitea#23475)
  test_env: hardcode major go version in use (go-gitea#23464)
  Push option bonus for PTC docs (go-gitea#23473)
  Lint Markdown pass
  Push to create docs (go-gitea#23458)
  Convert GitHub event on actions and fix some pull_request events. (go-gitea#23037)
  Remove wrongly added column on migration test fixtures (go-gitea#23456)
  Refactor branch/tag selector to Vue SFC (go-gitea#23421)
  add admin API email endpoints (go-gitea#22792)
  add user rename endpoint to admin api (go-gitea#22789)
  Add workflow error notification in ui (go-gitea#23404)
  Make branches list page operations remember current page (go-gitea#23420)
  fix markdown lint issue (go-gitea#23457)
  ...
@silverwind
Copy link
Member

silverwind commented Mar 15, 2023

generate svg by function, instead of template: avoid using vue runtime compiler, then Gitea works with disabled unsafe-eval CSP.

Does that mean we can drop the @vue/compiler-sfc dependency? Is that the runtime compiler or is it something else?

@wxiaoguang
Copy link
Contributor Author

According to Vue document, the runtime compiler could be tree-shaked but I haven't confirmed it and haven't tested how @vue/compiler-sfc affectes the packing. According to https://github.com/vuejs/core/tree/main/packages/compiler-sfc :

Note: as of 3.2.13+, this package is included as a dependency of the main vue package and can be accessed as vue/compiler-sfc. This means you no longer need to explicitly install this package and ensure its version match that of vue's. Just use the main vue/compiler-sfc deep import instead.

There were still some unclear points to me, so I chose to keep them as before. 😂

If you have ideas about how to continue to improve, feel free to do it and I'd like to learn more.

@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/done This PR has enough approvals to get merged. There are no important open reservations anymore. 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.

Branch selector disappear immediately (Uncaught EvalError: 'unsafe-eval' is not an allowed)
8 participants