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 #27666 Inconsistent use of window.requestAnimationFrame v requestAnimationFrame #30341

Closed
wants to merge 4 commits into from

Conversation

skyclouds2001
Copy link
Contributor

@skyclouds2001 skyclouds2001 commented Nov 17, 2023

Description

Motivation

Additional details

Related issues and pull requests

#27666

note that this may not a fix to #27666, as the issue creator suggest that use requestAnimationFrame instead of window.requestAnimationFrame, but in my view below, it is better to use window.requestAnimationFrame, reasons are below

and @blindman67 did not give further response anymore

@github-actions github-actions bot added the Content:WebAPI Web API docs label Nov 17, 2023
Copy link
Contributor

github-actions bot commented Nov 17, 2023

Preview URLs

(comment last updated: 2024-03-13 10:07:29)

@skyclouds2001
Copy link
Contributor Author

@blindman67 I wonder if this contribution fix your issue, as I am not quite understand your meaning

@blindman67
Copy link

@skyclouds2001 No not fixed.

window is the default this and its use in the examples is redundant. IE window.requestAnimationFrame is the same reference as requestAnimationFrame

In the example code the use of window is inconsistent (across all examples). It is used with window.requestAnimationFrame but not with window.document.createElement or new window.Date() and many more window properties used in the examples.

@skyclouds2001
Copy link
Contributor Author

skyclouds2001 commented Nov 18, 2023

@skyclouds2001 No not fixed.

window is the default this and its use in the examples is redundant. IE window.requestAnimationFrame is the same reference as requestAnimationFrame

In the example code the use of window is inconsistent (across all examples). It is used with window.requestAnimationFrame but not with window.document.createElement or new window.Date() and many more window properties used in the examples.

Well, @blindman67 I understand what you mean now completely

And my opinion is that, whether we use window.xx or xx might can be judged by following reasons:

  • whether the variable is in Web API standard or in ECMAScript standard (if in ECMAScript standard, it shouldn't relate with window)
  • whether the variable is defined as a property on Window interface or just simply expose to global scope (in main context it is the window) (if just simply expose to global scope, it shouldn't visit via with window)
  • whether the variable is a well-known symbol (if well-known enough, can simply by avoiding visiting via window)
  • wether other developers may confuse the variable as a custom variable (if the name of the variable is commonly used in other cases, it'd better to use it via window)

For example:

  • prefer using document instead of window.document, since document property is well-known enough and develpors wouldn't confuse it as a custom variable
  • prefer using Date instead of window.Date, since Date is exposed to global scope not defined on Window interface and Date is a ECMAScript standard, not a Web API standard
  • prefer using window.print() instead of print(), since print is a commonly used symbol and may be comfused by other developers

Here I think use window.requestAnimationFrame() is better

@skyclouds2001
Copy link
Contributor Author

see also #10573 (comment) and #30329 (comment)

@bsmth bsmth changed the title Fix #27666 Fix #27666 Inconsistent use of window.requestAnimationFrame v requestAnimationFrame Nov 20, 2023
@skyclouds2001
Copy link
Contributor Author

Beside this, I agree that it is needed to consistent the usage of window.requestAnimationFrame or requestAnimationFrame

@skyclouds2001
Copy link
Contributor Author

@blindman67 would you offer your idea on whether using window.requestAnimationFrame or requestAnimationFrame is better

@skyclouds2001 skyclouds2001 marked this pull request as ready for review November 28, 2023 14:02
@skyclouds2001 skyclouds2001 requested a review from a team as a code owner November 28, 2023 14:02
@skyclouds2001 skyclouds2001 requested review from Elchi3 and removed request for a team November 28, 2023 14:02
@teoli2003
Copy link
Contributor

Prefixing requestAnimationFrame() with window prevent to directly copy-paste the snippet into a worker code. Using self or nothing doesn't have this problem (and they still work on the Window global scope).

I have no strong feelings about these two solutions (self or nothing), though we should uniformize–for sure on a given page, likely everywhere– and I don't see a benefit of self..

cc/ @wbamberg @Elchi3 that may have some extra insight here.

@github-actions github-actions bot added the size/xs [PR only] 0-5 LoC changed label Mar 13, 2024
@bsmth bsmth added the needs decision The task needs consensus through discussion label Mar 15, 2024
@skyclouds2001 skyclouds2001 deleted the 27666 branch March 30, 2024 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebAPI Web API docs needs decision The task needs consensus through discussion size/xs [PR only] 0-5 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants