-
Notifications
You must be signed in to change notification settings - Fork 20.6k
Core: Use window.setTimeout & friends instead of global equivalents #2397
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
Conversation
@@ -0,0 +1,3 @@ | |||
define(function() { | |||
return window.clearInterval; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aaam, why do we put those functions in different modules?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because they're variables so they have to be in different modules. It's the same with other window globals like document
, documentElement
as well as methods like toString
, indexOf
etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, not sure how you'd otherwise reference setTimeout
etc. in other modules if it wasn't a value returned from a module but just attached to it somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If they were always used together you could put them in a single module and reference them from an object it returned. However that is probably less compressible and slower since the property name won't be minified the way a variable is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However that is probably less compressible and slower since the property name won't be minified the way a variable is?
Easy to check :)
@timmywil could you provide a consult on amd here, i understand why we do this for toString
or indexOf
but defining document
and documentElement
? Is that some sort of convention or convenience?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true, but mitigable by two means: resetting style & innerHTML before use (which we actually do, but can't enforce), and using distinct variable names. At any rate, it's off topic for this discussion so I'll either forget about it or open a PR eventually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the document
module would increase the size by 26 bytes, though so I'm not really into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's off topic for this discussion so I'll either forget about it or open a PR eventually.
No-no, lets not forget! If we don't do it, somebody else might - #2405
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's used in just one file but in many places
By "place" i meant "file" :-), created - #2407
If no one opposes, I'll land it tomorrow & create a separate PR for removing some |
I had to change those modules to regular |
Ugh. |
I bet we can workaround that, but i also bet, that we don't want to workaround that. That is what i meant by "complication" of code, when we use such convenience style of variable initialization. |
Fixes jquerygh-2501 Closes jquerygh-2504 Refs jquerygh-1950 Refs jquerygh-1949 Refs jquerygh-2397 Refs jquerygh-1537 Refs 842958e
Fixes jquerygh-2133 Fixes jquerygh-2501 Closes jquerygh-2504 Refs jquerygh-1950 Refs jquerygh-1949 Refs jquerygh-2397 Refs jquerygh-1537 Refs 842958e
Fixes jquerygh-2133 Fixes jquerygh-2501 Closes jquerygh-2504 Refs jquerygh-1950 Refs jquerygh-1949 Refs jquerygh-2397 Refs jquerygh-1537 Refs jquerygh-2504 Refs 842958e
Fixes jquerygh-2133 Fixes jquerygh-2501 Closes jquerygh-2504 Refs jquerygh-1950 Refs jquerygh-1949 Refs jquerygh-2397 Refs jquerygh-1537 Refs jquerygh-2504 Refs 842958e
Fixes gh-2177
+28 bytes.
An alternative to #2391 (+60), #2392 (+51) & #2394 (+40).