Skip to content

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

Merged
merged 1 commit into from
Jun 17, 2015

Conversation

mgol
Copy link
Member

@mgol mgol commented Jun 16, 2015

Fixes gh-2177

+28 bytes.

An alternative to #2391 (+60), #2392 (+51) & #2394 (+40).

@@ -0,0 +1,3 @@
define(function() {
return window.clearInterval;
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member

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?

Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@markelog

If we decide that this in fact overkill, that we should definitely rethink this change, since this var is used only in one place

It's used in just one file but in many places. Removing the var saves 7 bytes, though...

Copy link
Member Author

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.

Copy link
Member

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

Copy link
Member

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

@mgol
Copy link
Member Author

mgol commented Jun 16, 2015

If no one opposes, I'll land it tomorrow & create a separate PR for removing some var modules.

@mgol mgol force-pushed the window-settimeout4 branch from 70aa226 to 219c749 Compare June 17, 2015 10:31
@mgol mgol merged commit 219c749 into jquery:master Jun 17, 2015
@mgol mgol deleted the window-settimeout4 branch June 17, 2015 10:31
@mgol
Copy link
Member Author

mgol commented Jun 17, 2015

I had to change those modules to regular window.* uses after all as using modules caches those methods forever, preventing Sinon from mocking them & breaking many many effects tests.

@dmethvin
Copy link
Member

Ugh.

@markelog
Copy link
Member

I had to change those modules to regular window.* uses after all as using modules caches those methods forever, preventing Sinon from mocking them & breaking many many effects tests.

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.

mgol added a commit to mgol/jquery that referenced this pull request Jul 28, 2015
mgol added a commit to mgol/jquery that referenced this pull request Jul 28, 2015
mgol added a commit to mgol/jquery that referenced this pull request Jul 28, 2015
@mgol mgol removed the Needs review label Jul 31, 2015
mgol added a commit to mgol/jquery that referenced this pull request Aug 16, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

6 participants