-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixing toJson #4
Comments
I think that function is badly named, since there is already JSON.parse(), causing confusion to what this toJson does. Better would be toBackendJson() or something. In the cases where you don't want the hacks you can just use JSON.parse instead, so why make a new one or add parameters? |
Well of course you are right. It would basically be a function that wraps |
Or actually, |
I agree with @nickjanssen that this method is named badly. It's not at all converting anything to JSON. It's the other way around. It's parsing a JSON string into a JavaScript object. So it should rather be something like |
This went hand in hand with the |
sofa.Util.toJson
still handles stupid backend response. Despite from the fact that the backend should be fixed accordingly, it also introduces a side effect as you can see in line348
https://github.com/sofa/sofa-core/blob/master/src/sofa.util.js#L357-L371.Any updates on the backend handling?
We should make the
toJson
method behave like one would expect when using atoJson
method. Therefore I would recommend either introducing a separate method that usestoJson
and adds the side effect, or introducing a parameter to control wether we wanttoJson
behave like ass or not.@cburgdorf @nickjanssen thoughts anyone?
Background is that I need this method in
sofa.HttpService
. Of course, the places where our http service is used only calls our backend, which means calling the currenttoJson
on top of that with the side effect would actually be the right thing.But to do things "right" we should expect that one can use the http service for other endpoints too!
The text was updated successfully, but these errors were encountered: