greencatsoft / scalajs-angular

AngularJS Binding for Scala.js
Apache License 2.0
252 stars 42 forks source link

Location.url() returns js.Object instead of String #66

Closed easel closed 8 years ago

easel commented 9 years ago

I got some UBE's attempting to use Location.url(). Apparently the return result is an object, not a string.

mysticfall commented 8 years ago

Could you give me a reference about the suggested change?

The documentation specifies the return type of the mentioned method to be of string type, and I just did a quick test myself but was unable to reproduce the problem.

Please let me know if I am missing something here. And sorry for the late response again!

easel commented 8 years ago

Here's the issue:

https://github.com/angular/angular.js/blob/v1.4.x/src/ng/location.js#L392

When you call .url("someNewThing") it modifies the url and then returns this, which isn't a string. The "getter" does return a string. Perhaps the better ascription would be String | Location?

mysticfall commented 8 years ago

Thanks for the confirmation. And I agree that the suggested change would make it even clearer. Could you modify the PR using the union type?

easel commented 8 years ago

Yeah will do.

easel commented 8 years ago

Ok I think this is actually a better solution. I've split the two methods by making "url" non-optional. So far, it seems to work as expected =p

mysticfall commented 8 years ago

Oh, thanks! It's way better, indeed :) By the way, isn't there a typo that the url parameter missing type definition?

easel commented 8 years ago

Yeah good catch, I guess I forgot to push the branch after fixing that. Should be good to go now.

mysticfall commented 8 years ago

Merged now. Thanks for the contribution!

easel commented 8 years ago

Thanks for the merge!