refactor(ngResource): Use core encode methods in ngResource

[nicholasserra]

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)
Core package and ngResource both define methods for encodeUriQuery and encodeUriSegment.
ngResource's methods are out of date, as changes were made to core encodeUriQuery over a year ago.

What is the new behavior (if this is a feature change)?
Refactor the usage of these methods in ngResource by removing the duplicate methods, and using the core methods instead.

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)

[nicholasserra]

Looks like a single saucelabs job timed out :P

[Narretz]

This is not a just a refactoring, this exposes these two functions on the angular global, which is something we don't do lightly

[nicholasserra]

You're correct. This is my first patch contributed, so I wasn't sure if this was a big deal or not. Although I do see various utility methods exposed publicly. The nature of the encoding methods makes me feel that they could be useful as public methods.

Interested to hear your thoughts, or general conditions on public methods on the core module. Thanks for the feedback :)

[gkalpak]

As mentioned by @Narretz, we try to avoid exposing internal helpers/utility functions as public.
On the other hand, it has been bugging me for sometime, that we are using two identical(?) implementation of the same functions. Apart for being WET (isn't that the opposite of DRY), there is the danger of those getting out of sync. E.g. 3625803 has fixed the core version only. Does it mean it should not be included in ngResource version or was it just an oversight ? Who knows...

To expose or not to expose ? 😕

BTW, if we decided to expose them, I would suggest $$ prefixing the keys to declare them as Angular-private.

[nicholasserra]

Yeah, it was bugging me too. Mainly because I want to propose some changes around some of the encodings, and it seemed wrong to make the identical methods diverge further.

@gkalpak Can you point me to an example in the repo of the $$ private prefix on methods? If it is accepted that we go this route, i'll follow your suggestion and fix those.

Thanks!

[nicholasserra]

Also wondering why this saucelabs test keeps timing out on me :-/

[gkalpak]

I have restarted the build 4 times and the tests keep failing (on Safaris). I wonder if it's a real issue (something to do with the difference introduced in 3625803).

WRT $$, I mean instead of angular.encodeUriSegment = encodeUriSegment do angular.$$encodeUriSegment = encodeUriSegment. $$-prefixed properties/methods/services are considered "Angular-private" and users are not supposed to touch them (or suffer the consequences when we break them without notice 😁).

[nicholasserra]

Usually when I see saucelabs errors like this, it's a fluke. Odd that it's consistent. Can I run those selenium tests locally? grunt test:unit passes, but I don't think that runs integration tests. I'll try to see if I can get it running locally.

I'll update the PR with those pseudo private prefixes too.

[gkalpak]

grunt test:unit passes, but I don't think that runs integration tests

It's the unit tests that fail on Travis. Other tests pass.
The failing browsers are Safari 8.0.8 (Mac OS X 10.10.5) and Safari 9.0.0 (Mac OS X 10.11.0).

[nicholasserra]

Looks like master is actually failing.

[nicholasserra]

Pseudo private methods are now using $$. I'll wait until master is green and rebase this.

[nicholasserra]

And now we're passing :)

[nicholasserra]

@gkalpak Any new thoughts on this? Appreciate the input, thanks!

[lgalfaso]

I do not like the idea of exposing these function... why not just move these functions to a different file and include this file when building angular core and ngResources ?

I know that sharing files between modules is something we currently not do, I just do not like the idea of exposing more helper functions.

[gkalpak]

I know that sharing files between modules is something we currently not do

We do it with the loader IIRC, but yeah, we are not doing it with a bunch of other stuff.
If we start going down that path, I'm afraid we are going to complicate our build process for little gain.

I just do not like the idea of exposing more helper functions

I'm not thrilled about it either, but nor do I like the current situation that we duplicate code.
On the other hand these won't be "public" helpers, so we won't have to keep them backwards compatible. Do you have other concerns, besides that ?

Just to be clear, these helper functions, won't be public. They won't be documented and will be $$-prefixed, so breaking them shouldn't be an issue. If someone relies on undocumented, private $$ features, they have it coming :)
BTW, it won't be the first time we expose undocumented $$ stuff to modules. Off the top of my head, we have a couple of those exposed for ngAnimate (not as properties on window.angular, but as services).

So, considering the options:

1. Have duplicate implementations of the same functions (this is what happens now)
2. Expose the functions as private, undocumented helpers on window.angular (this is what this PR does)
3. Move functions to a separate file and include that file to all modules that need it

I don't feel too strongly about either, but slightly favor the first two over the 3rd one.

In fact, if we decide to go with the 2nd option (private helpers), we could introduce window.angular.$$privateHelpers and attach private helpers there (as a means to be even more explicit that those are private), but I'm definitely not too happy with that option either.

Another approach would be making those injectable (again behind a $$-prefix), which is a little more involved, but doable.

[nicholasserra]

My thoughts are the same as above ^

Seems like adding build complexity is overkill because these are pseudo private methods.

[lgalfaso]

Ok, lets go with the current approach. We can always change it later :)

[nicholasserra]

Awesome :) Excited to get some more patches in related to this issue.

[gkalpak]

@lgalfaso, with "current approach" did you mean the one currently in effect or the one proposed in the PR ? (I assume the former, right ?)

[gkalpak]

/ping @lgalfaso

[nicholasserra]

Hello everyone. Hoping for a decision on this, as I have some more patches planned around the encode methods. Thanks :)

[lgalfaso]

@gkalpak by current approach I meant the one in the PR

[gkalpak]

I was about to merge this, when I realized that this is a potential (minor) breaking change.
Although it shouldn't matter in practice (since both the encoded and the unencode ; would be interpreted identically from the server), it could break some tests (where for example $httpBackend was set up to expecte an encode ;, but the request is made to the URL with unencoded ;).

BC or fix?

[petebacondarwin]

If there is a possibility of a BC then let's just put this in 1.6 and document the BC.
We don't want to have to muck about with potential G3 breakages because of this small improvement.

[nicholasserra]

I thought of this as a fix. This is an example of why the duplicate methods were an issue. A PR was merged with a patch for replacing semicolons in the encodeUriQuery method, but the patch only made the change in one of the methods, in this case the core encodeUriQuery method. It probably should have been done in both instances.

My (this) patch keeps the correct core version of the encodeUriQuery method.

[gkalpak]

@nicholasserra, the problem is that since v1.3.0-beta.18 $resource has been behaving in a specific way. With this fix, this way will change and could result in broken tests. It is something to be avoided in a patch release. Updating from 1.5.x to 1.5.{x+1} should be a drop-in replacement.

[nicholasserra]

Understood. 1.6 sounds good.

[gkalpak]

I tweaked it a bit, added a test and landed.
But forgot to add a breaking change notice in 2456ab6 😕