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.

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

Jump to bottom

Remove jQuery ready usage #23858

Merged
merged 4 commits into from
Apr 1, 2023
Merged

Remove jQuery ready usage #23858

merged 4 commits into from
Apr 1, 2023

Conversation

silverwind
Copy link
Member

Replace it with equal function of our own and enable the eslint rule to forbid future usage.

@silverwind silverwind added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Apr 1, 2023
@silverwind silverwind added this to the 1.20.0 milestone Apr 1, 2023
@silverwind silverwind added the topic/ui Change the appearance of the Gitea UI label Apr 1, 2023
silverwind
silverwind commented Apr 1, 2023
View reviewed changes
web_src/js/utils/dom.js Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 1, 2023
@silverwind silverwind added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Apr 1, 2023
@wxiaoguang
Copy link
Contributor

I guess we do not need DOMContentLoaded.

Because the HTML code looks like this:

<html>
<body>
  <div page>
      ....
  </div>

  <script index.js>
</body>
</html>

When index.js gets executed, all necessary DOM elements are already there (ready), we do not need to wait for the remaining </body>.

So, how about making code just call every init function directly in index.js?

@silverwind
Copy link
Member Author

silverwind commented Apr 1, 2023 •
edited
Loading

Are you 100% certain that document.readyState is guaranteed to not be loading at this point? I'm trying to play it safe here.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 1, 2023 •
edited
Loading

Are you 100% certain this will work? I'm trying to play it safe here.

99.9999% sure.

document.readyState is guaranteed to not be loading at this point

It doesn't matter, there won't be any new content, because this <script index.js> is the last DOM content.

DOMContentLoaded / $.ready is only used for 2 purposes:

  • Some historical broken browser (as old as IE6-9), if the DOM is not ready, many operations on DOM cause page crash.
    It's never been a problem for modern browsers.
  • For JS code which is ahead of its related DOM elements, like <script>$.ready</script><div the-related></div>
    It's not our case.

In fact, we already have a lot of code operating the DOM directly without DOM ready, for example, the "Clone" / "DiffView" inline <script>.

I think it's safe to simplify the code in 1.20, we have enough time to eat our own dogfood.

@silverwind
Copy link
Member Author

Ok, then let's try it. If there are regressions, we should be able to notice in time.

@silverwind
Copy link
Member Author

Tried it, got this error:

image

So I think we can not do it, at least not without further refactors.

@silverwind
Copy link
Member Author

Also, vue components are broken without the callback:

image

@wxiaoguang
Copy link
Contributor

Hmm .... it's caused by <script module> .....

Tricky .... I have a feeling that it's not DOMContentLoaded's problem, but the <script module> is wrong, it wrongly defers its execution.

So to keep things simple, let's keep the DOMContentLoaded, I will take a look at the <script module> problem later.

@silverwind
Copy link
Member Author

silverwind commented Apr 1, 2023 •
edited
Loading

Yeah. PR is ready as-is.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 1, 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 Apr 1, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 1, 2023
@techknowlogick techknowlogick enabled auto-merge (squash) April 1, 2023 22:16
@techknowlogick techknowlogick enabled auto-merge (squash) April 1, 2023 22:16
@techknowlogick
Copy link
Member

🤖 🎺

@delvh delvh changed the title Remove jQuery ready usage Remove jQuery ready usage Apr 1, 2023
@techknowlogick techknowlogick merged commit ae36113 into go-gitea:main Apr 1, 2023
@silverwind silverwind deleted the no-ready branch April 2, 2023 07:16
@lunny lunny removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 2, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 3, 2023
@zjjhot
Merge remote-tracking branch 'upstream/main'
a6741d8 
* upstream/main:
  [skip ci] Updated translations via Crowdin
  Update JS deps (go-gitea#23853)
  Added close/open button to details page of milestone (go-gitea#23877)
  Check `IsActionsToken` for LFS authentication (go-gitea#23841)
  Prefill input values in oauth settings as intended (go-gitea#23829)
  Display image size for multiarch container images (go-gitea#23821)
  Use clippie module to copy to clipboard (go-gitea#23801)
  Remove assertion debug code for show/hide refactoring (go-gitea#23576)
  [skip ci] Updated translations via Crowdin
  Remove jQuery ready usage (go-gitea#23858)
  Fix JS error when changing PR's target branch (go-gitea#23862)
  Improve action log display with control chars (go-gitea#23820)
  Fix review conversation reply (go-gitea#23846)
  Improve home page template, fix Sort dropdown menu flash (go-gitea#23856)
  Make first section on home page full width (go-gitea#23854)
  [skip ci] Updated translations via Crowdin
  Fix incorrect CORS failure detection logic (go-gitea#23844)
@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.
Reviewers

@delvh delvh delvh approved these changes

@lunny lunny lunny approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. 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
Milestone
1.20.0
Development

Successfully merging this pull request may close these issues.
6 participants
@silverwind @wxiaoguang @techknowlogick @lunny @delvh @GiteaBot