Fixing ng-jq issue with getNgAttribute

[mboudreau]

The unit tests wasn't able to catch this unless it's on a 'fresh' angular page with ng-jq already added before angular bootstrapping. I've removed the need for jq() to rely on getNgAttribute and instead using element.getAttribute function with the found prefix.

Could use some direction on how to test ng-jq more thoroughly so this doesn't happen again in the future.

This fixes issue #11016.

[mboudreau]

@Narretz I agree that we should add e2e tests, but I find the current way of Angular doing e2e tests fairly confusing to understand and I haven't found much documentation on such. Plus, I'm not sure how to setup the test in a fresh page that already has ng-jq predefined before Angular loads.

Any direction in this would be very helpful.

[Narretz]

Maybe @caitp can help, she refactored the e2e a few months ago. @caitp do you think if it makes sense to e2e test this? And is this possible with the current setup?

[caitp]

First of all, I think this is the totally wrong approach to solving this issue.

The issue is about using the jquery api within getNgAttribute(), so all that needs to be done is to rewrite getNgAttribute() to not use the jquery api, which is pretty trivial.

For e2e testing, there is a sample test in test/e2e --- you basically just need to create a new fixture (give it a name, like jquery-binding or something), similar to the sample fixture, and adapt the sample test to fetch the new fixture, and write some tests to make assertions about the new fixture doing what you expect it to do.

There isn't much of a guide, but it should be simpler than writing docs e2e tests. I am happy to review and give pointers on writing the test(s) next week.

[gkalpak]

Indentation...
(Again... 😸)

[mboudreau]

god damn spaces...

[mboudreau]

Thanks @caitp, I'll definitely add that in. I would be keen to change the getNgAttribute function to not use jquery api and I agree with it as well since it's a dependency on an object that we don't know when it'll be available. However, I didn't want to change it in case there's a particular use case needed for it and I don't know the angular project enough to make my own decision on this.

@Narretz Will definitely add e2e tests, but in the meantime, can we merge this to fix the issue?

[gkalpak]

@mboudreau: Since you seem to have fixed getNgAttribute() to not require jqLite/jQuery, should we revert back to the more concise variation that uses getNgAttribute(el, 'jq') ?

[mboudreau]

@gkalpak I was thinking about it, but since I am already searching for ng-jq through the prefixes, I didn't see the point of doing another loop through all the prefixes if I already know it. I'll leave it up to you if that's a good idea, but figured it was only 1 loop through the prefixes that's needed, not 2.

[gkalpak]

@mboudreau, either way is fine, I guess. It's micro-optimizing performance vs micro-optimizing readability, so it's just a matter of taste :)
Leaving it up to you !

[mboudreau]

@gkalpak You make a good point, but it's not that hard to understand, just not using the helper function to make it 'easier'. I'd take a readability hit for a performance gain on that unless someone on the angular team has a really big issue with it.

[Narretz]

I'll see it like @gkalpak - I prefer getNgAttribute, but I don't insist on it.
We really should have the tests for merging this, though. I can look into it this week.

[mboudreau]

@Narretz @gkalpak @caitp I'm willing to try to get e2e tests going on this, but going to need some direction first. Either that, or at the very least, merge this into master so it fixes the bug that was discovered by this PR.

[Narretz]

I'm looking into e2e test now. We can't merge this atm - we raelly only want to merge with tests.

[Narretz]

New PR in #11182