feat(jqLite): implement jqLite(f) as alias to jqLite(document).ready(f) (also: deprecate the latter in the second commit) #15237

mgol · 2016-10-09T22:55:59Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Feature

What is the current behavior? (You can also link to an open issue here)
jqLite(callback) is not supported.

What is the new behavior (if this is a feature change)?
jqLite(callback) is an alias to jqLite(document).ready(callback). The latter is now deprecated.

Does this PR introduce a breaking change?
No.

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:

dcherman · 2016-10-09T23:28:44Z

src/jqLite.js

+
+  function trigger() {
+    if (fired) return;
+    fired = true;


I know the old code didn't do this, but shouldn't we just remove the event handlers rather than setting a flag?

Good idea. I jut copied it without analysing the contents too much. 😄

Since we are wiring up both DOMContentLoaded and load events, is it not possible that they will both be triggered even if we remove the event handlers from inside the trigger function?
In other words, if the browser queues up both handlers before either handler has run, then removing the handlers inside one of the handlers would not prevent the other from running.

Doubt it. I'd imagine it would be considered a browser bug if any browsers behaved like that, otherwise you also couldn't add a handler for load in DOMContentReady which is almost certainly expected to work.

There's also precedent since this is how jQuery does it, so it's pretty battle tested.

I added a "refactor" in a separate commit.

petebacondarwin

LGTM - we can do the refactor (if it works) of removing the event handlers in another PR later.

petebacondarwin · 2016-10-10T05:13:55Z

src/jqLite.js

+
+  function trigger() {
+    if (fired) return;
+    fired = true;


Since we are wiring up both DOMContentLoaded and load events, is it not possible that they will both be triggered even if we remove the event handlers from inside the trigger function?
In other words, if the browser queues up both handlers before either handler has run, then removing the handlers inside one of the handlers would not prevent the other from running.

petebacondarwin · 2016-10-10T05:18:10Z

src/jqLite.js

@@ -76,7 +76,7 @@
 * - [`parent()`](http://api.jquery.com/parent/) - Does not support selectors
 * - [`prepend()`](http://api.jquery.com/prepend/)
 * - [`prop()`](http://api.jquery.com/prop/)
- * - [`ready()`](http://api.jquery.com/ready/)
+ * - [`ready()`](http://api.jquery.com/ready/) (_deprecated_, use `angular.element(callback)` instead of `angular.element(document).ready(callback)`)


document removal in 1.7?

To be discussed; see #15221.

Conclusion: no removal note.

gkalpak · 2016-10-10T06:45:27Z

src/jqLite.js

@@ -288,6 +288,8 @@ function JQLite(element) {

  if (argIsString) {
    jqLiteAddNodes(this, jqLiteParseHTML(element));
+  } else if (isFunction(element)) {


Couldn't this go above the instanceof JQLite check?

I didn't want to delay the most common path and jQuery has this check below the selector code. Maybe with JITs it's not a problem.

It's probably not a problem, but it can't do much damage either (how many times would one call jqLite(fn)?).
So if jQuery has it this way, let's leave it as is.

jQuery is slightly different as it handles nodes after the function.

Maybe we should apply this here as well? That would only require an additional check for nodeType.

OK, jQuery does it as it's done here, I read the code incorrectly.

gkalpak · 2016-10-10T06:47:57Z

src/jqLite.js

+    window.document.addEventListener('DOMContentLoaded', trigger);
+
+    // Fallback to window.onload for others
+    window.addEventListener('load', trigger);


By using addEventListener (here and above) doesn't it mean the people can't use .triggerHandler()?
It could break apps (and even more likely tests).

It does but we no longer have an underlying element and that's what jQuery uses so this is aligning with jQuery.

OK. Should we document it as breaking change then?

Actually, it shouldn't be possible to exercise the difference from a test, since you can't stop the DOMContentLoaded/load events. So, it's fine, I guess.

Yes, let's document it. I need to think about the phrasing that will make it clear it's an edge case.

...or do you mean it's not a breaking change in your last commit?

OK, I'll assume there's nothing to do here. :D

Yeah, I meant let's not consider it a breaking change 😃

gkalpak

LGTM

gkalpak · 2016-10-10T08:55:23Z

src/jqLite.js

@@ -288,6 +288,8 @@ function JQLite(element) {

  if (argIsString) {
    jqLiteAddNodes(this, jqLiteParseHTML(element));
+  } else if (isFunction(element)) {


It's probably not a problem, but it can't do much damage either (how many times would one call jqLite(fn)?).
So if jQuery has it this way, let's leave it as is.

gkalpak · 2016-10-10T08:57:09Z

src/jqLite.js

+    window.document.addEventListener('DOMContentLoaded', trigger);
+
+    // Fallback to window.onload for others
+    window.addEventListener('load', trigger);


OK. Should we document it as breaking change then?

gkalpak · 2016-10-10T08:58:41Z

src/jqLite.js

+    window.document.addEventListener('DOMContentLoaded', trigger);
+
+    // Fallback to window.onload for others
+    window.addEventListener('load', trigger);


Actually, it shouldn't be possible to exercise the difference from a test, since you can't stop the DOMContentLoaded/load events. So, it's fine, I guess.

jQuery has supported this form for a long time. As of jQuery 3.0 this form is the preferred one and all others are deprecated so jqLite(f) is now also supported. All internal invocations of jqLite(document).ready(f) (& equivalent) have been replaced by jqLite(f). Tests for these methods have been added as jqLite#ready had no explicit tests so far.

Use jqLite(fn) instead of jqLite(document).ready(fn).

We're not going to remove the aliases before jQuery does.

This change aligns jqLite with the jQuery implementation. Closes angular#15237

gkalpak · 2016-10-12T14:05:07Z

Still LGTM

jbedard · 2016-10-12T16:39:56Z

src/jqLite.js

+
+  // check if document is already loaded
+  if (window.document.readyState === 'complete') {
+    window.setTimeout(trigger);


Should this just be window.setTimeout(fn) since we aren't adding the listeners?

Good point, fixed.

It looks more & more similar to the jQuery code. :)

….ready(f)

This change aligns jqLite with the jQuery implementation. Closes angular#15237

mgol added type: feature component: jqLite labels Oct 9, 2016

mgol added this to the 1.7.0 milestone Oct 9, 2016

mgol self-assigned this Oct 9, 2016

googlebot added the cla: yes label Oct 9, 2016

mgol modified the milestones: 1.6.0-rc.0, 1.7.0 Oct 9, 2016

dcherman reviewed Oct 9, 2016

View reviewed changes

petebacondarwin approved these changes Oct 10, 2016

View reviewed changes

petebacondarwin reviewed Oct 10, 2016

View reviewed changes

gkalpak reviewed Oct 10, 2016

View reviewed changes

gkalpak approved these changes Oct 10, 2016

View reviewed changes

mgol force-pushed the ready branch from 455e4ba to 6ea385a Compare October 12, 2016 12:38

mgol added 4 commits October 12, 2016 14:46

refactor(jqLite): deprecate jqLite#ready

272b511

Use jqLite(fn) instead of jqLite(document).ready(fn).

docs(jqLite): remove the removal plan info for bind/unbind

7d2c5df

We're not going to remove the aliases before jQuery does.

chore(jqLite): remove the ready handlers instead of setting a flag

983bbf7

This change aligns jqLite with the jQuery implementation. Closes angular#15237

mgol force-pushed the ready branch from e833445 to 983bbf7 Compare October 12, 2016 13:14

jbedard reviewed Oct 12, 2016

View reviewed changes

fixup! feat(jqLite): implement jqLite(f) as alias to jqLite(document)…

8adaec4

….ready(f)

mgol closed this in beab3ba Oct 12, 2016

mgol deleted the ready branch October 12, 2016 17:33

ellimist pushed a commit to ellimist/angular.js that referenced this pull request Mar 15, 2017

chore(jqLite): remove the ready handlers instead of setting a flag

23d745f

This change aligns jqLite with the jQuery implementation. Closes angular#15237

feat(jqLite): implement jqLite(f) as alias to jqLite(document).ready(f) (also: deprecate the latter in the second commit) #15237

feat(jqLite): implement jqLite(f) as alias to jqLite(document).ready(f) (also: deprecate the latter in the second commit) #15237

Uh oh!

Conversation

mgol commented Oct 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgol Oct 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgol Oct 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gkalpak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gkalpak commented Oct 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mgol Oct 10, 2016 •

edited

Loading

mgol Oct 10, 2016 •

edited

Loading