Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

refactor(jQlite): Add the ability to use query selector without jQuery #15986

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

orafaelfragoso
Copy link

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

What is the current behavior? (You can also link to an open issue here)
angular.element('.my-selector') throws an error if jQuery is not included.

What is the new behavior (if this is a feature change)?
angular.element('.my-selector') now finds the element and wraps it in a jQlite (or jQuery) object.

Does this PR introduce a breaking change?
No

Please check if the PR fulfills these requirements

Other information:

angular.element threw an error if we tried to use query selector
directly without jQuery. I thought that this was a silly little
fix for the framework.

A lot of the projects that I've worked for
the past years are only including jQuery to use query selectors on
the app (I know that are options for this), and I'm only making this PR
because I think it's unnecessary to use
angular.element(document).find() to accomplish such a small thing and
to get the jQlite wrapper with the goodies.

Now we can do this, without jQuery:

angular.element('.something')

And get the same result.

@mgol
Copy link
Member

mgol commented May 15, 2017

There was extensive discussion about making .find(selector) work and we decided against it as it was impossible to defer to querySelectorAll in a simple way that will agree with jQuery (& common sense) due to a design bug of querySelectorAll with respect to running from non-document elements.

But these arguments don't extend to root-level selectors so I can see angular.element(selector) working in JQLite... I just want to make it clear we're not going to support .find(selector).

Thanks for the PR!

@mgol
Copy link
Member

mgol commented May 15, 2017

@rafaelfragosom Please make sure grunt test passes for you locally, there are some errors.

@orafaelfragoso
Copy link
Author

@mgol I agree, .find(selector) is too ugly for such a small DOM query.

I only want to be on the same page here. Are these changes wellcome or should I close this issue? Is querySelector an option or should I use a different approach? I can see the compatibility at almost 100% on caniuse.

Let me know so I can put some more effort on this.

Thank you.

@mgol
Copy link
Member

mgol commented Jul 26, 2017

@rafaelfragosom I consulted the team and we're OK with making angular.element(selector) work without jQuery but .find(selector) should still fail. As long as those conditions are met, we will accept a PR.

We'll need unit tests that confirm both of those conditions are met. Would you be willing to work on that?

src/jqLite.js Outdated
@@ -284,7 +284,7 @@ function JQLite(element) {
}
if (!(this instanceof JQLite)) {
if (argIsString && element.charAt(0) !== '<') {
throw jqLiteMinErr('nosel', 'Looking up elements via selectors is not supported by jqLite! See: http://docs.angularjs.org/api/angular.element');
return new JQLite(document.querySelector(element));
Copy link
Member

Choose a reason for hiding this comment

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

You need to use window.document. We don't assume browser globals are available globally, they have to be taken from window. You can also see that the Travis build failed because of that.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for letting me know.
I've been busy this entire time but I'll take the time to finish the PR.

Copy link
Author

Choose a reason for hiding this comment

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

I added the window call to it and rebased everything.

@orafaelfragoso orafaelfragoso force-pushed the angular-element-qs branch 4 times, most recently from ff4f3a1 to 389ecf4 Compare October 10, 2017 20:50
orafaelfragoso and others added 2 commits October 11, 2017 13:31
`angular.element` threw an error if we tried to use query selector
directly without jQuery. I thought that this was a silly little
fix for the framework.

A lot of the projects that I've worked for
the past years are only including jQuery to use query selectors on
the app (I know that are options for this), and I'm only making this PR
because I think it's unnecessary to use
`angular.element(document).find()` to accomplish such a small thing and
to get the jQlite wrapper with the goodies.

Now we can do this, without jQuery:

`angular.element('.something')`

And get the same result.
use the window to call document as instructed
Copy link
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

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

I think this needs refactoring, I added some comments.

Also, we need unit tests for all new functionality. Can you add some?

@@ -284,7 +284,7 @@ function JQLite(element) {
}
if (!(this instanceof JQLite)) {
if (argIsString && element.charAt(0) !== '<') {
throw jqLiteMinErr('nosel', 'Looking up elements via selectors is not supported by jqLite! See: http://docs.angularjs.org/api/angular.element');
return new JQLite(window.document.querySelector(element));
Copy link
Member

Choose a reason for hiding this comment

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

This is not a good place to add this logic as it'd mean new angular.element(selector) wouldn't work. This whole if should just be removed and the logic should me moved down.

Copy link
Member

Choose a reason for hiding this comment

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

Also, note it should use querySelectorAll, not querySelector as we want to select all matching elements, not one.

@mgol
Copy link
Member

mgol commented Mar 27, 2018

@rafaelfragosom Hey, are you still interested in finishing this PR?

@mgol
Copy link
Member

mgol commented Jan 26, 2019

We're now in LTS mode so no new features are accepted. Changing the milestone.

@mgol mgol modified the milestones: Purgatory, 1.7.x - won't fix Jan 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants