angular.dart icon indicating copy to clipboard operation
angular.dart copied to clipboard

fix DateTime json encode serialisation

Open giovannicandido opened this issue 11 years ago • 12 comments

This fix the problem #1412 Is not complete, because I trying to find a way for the user customise the jsonEncodable but I can't find a way that do not changes the signature for Function request = (HttpResponseConfig config). If I inject the configuration it can't be accessed in the method because this is not in the scope. One solution should create a static factory in other class.

Review on Reviewable

giovannicandido avatar Aug 31 '14 18:08 giovannicandido

I try a lot of things to inject the function for serialisation and change at runtime:

  • Inject in the function
  • Static Factories
  • globalFunction

No success. I think is because once created, the Interceptor do not change, but not sure, maybe my tests are wrong.

I will try this update later.

giovannicandido avatar Aug 31 '14 19:08 giovannicandido

Thanks for this PR.

My first reaction is "Why is DateTime special?". Hint: it isn't special -- but apps would love a generic JSON encoder that "just works" for anything, so I would envision this list growing dramatically.

But, http.dart is the wrong place for this change -- it highlights an erroneous coupling between Http and JSON. Instead, have a new class 'JsonParser' that exposes methods for encoding and decoding data. Inject that class into Http using DI.

Then, create two implementation of that class -- one uses the default JSON implementation which is fast and stupid. The second will inspect the data and have these hooks for Dates and other serializable classes. Apps can then pick their JsonParser implementation.

jbdeboer avatar Sep 03 '14 00:09 jbdeboer

I'm trying to decouple things without introducing a break change in the interceptor and having no success. How could I inject something in the response function, I try inject in the class but the function is in other scope and can't access the class properties

Atenciosamente, Giovanni Cândido da Silva

Em 02/09/2014, às 21:54, James deBoer [email protected] escreveu:

Thanks for this PR.

My first reaction is "Why is DateTime special?". Hint: it isn't special -- but apps would love a generic JSON encoder that "just works" for anything, so I would envision this list growing dramatically.

But, http.dart is the wrong place for this change -- it highlights an erroneous coupling between Http and JSON. Instead, have a new class 'JsonParser' that exposes methods for encoding and decoding data. Inject that class into Http using DI.

Then, create two implementation of that class -- one uses the default JSON implementation which is fast and stupid. The second will inspect the data and have these hooks for Dates and other serializable classes. Apps can then pick their JsonParser implementation.

— Reply to this email directly or view it on GitHub.

giovannicandido avatar Sep 03 '14 10:09 giovannicandido

Inject a "JsonParser" class into the HttpInteceptors class and then call a method on JsonParser from the response function.

jbdeboer avatar Sep 03 '14 20:09 jbdeboer

@jbdeboer Thanks! In my first try of implementation before the pull request, I make the mistake to try inject the instance in the DefaultTransformDataHttpInterceptor, and also the mistake of try to access a instance field in the request function, it need to be static in this case, otherwise is not in the scope of the method.

Now I'm working in the deserialization of DateTime Strings.

giovannicandido avatar Sep 04 '14 12:09 giovannicandido

I think is feature complete.

  • Provides a JsonParser class that can be injected to customize the behavior of serialization and deserialization for edge cases.
  • Default do serialize and deserialize DateTime ISO8601 dates, as the Dart SDK does provide a default implementation for dates. Now it just works.
  • Follow the SDK JSON.encode and JSON.decode signature.

Just one question. It needs documentation, where I could write that?

giovannicandido avatar Sep 04 '14 14:09 giovannicandido

I think the parser of date need more proper test, because ISO 8601 standard is a little more flexible, the regular expression used is functional, but do not cover some cases.

Is hard to cover all possible cases. But I think worth work more on that.

giovannicandido avatar Sep 06 '14 19:09 giovannicandido

Gentleman, what do you think about creating a new module "converter" with the JsonParser, it can be used in the json formatter as well in other places. Naming is a difficult art :-)

giovannicandido avatar Sep 06 '14 20:09 giovannicandido

The new module lib/converter/module.dart is ready to push (name is not final and with lots of new tests).

The are two questions that cross my mind:

  1. Other places use JSON.encode like the json formatter. I think is better align the behavior and use the module in this places as well, otherwise the developer will feel stranger that http encondes DateTime and formatter don't (and he is not able to custom that) screen shot 2014-09-06 at 20 23 44
  2. I think is better use encode, decode direct. Instead of everywhere
JSON.encode(d, toEncodable: _jsonParser.toEncodable );
JSON.decode(d, reviver: _jsonParser.reviver);

Use

JsonParser.decode(d);
JsonParser.encode(d);

And default do use the encodable and reviver functions

Advantages:

  1. More concise
  2. Developer can override the complete signature to not use encodable and reviver functions for crazy performance reasons.
  3. Open space for XML and other formats (In this case the class name needs change)
  4. Developer can do crazy stuff like changing the serialisation result at processing time or do whatever. I don't have a good use case for that, but who knows, maybe send a copy of all data to another application (could be done with http interceptors), maybe choosing a protocol by the content or criptographing the payload (SSL is better I know), or compressing (it can be done at http level), I'm running out of options :-)

Disadvantages:

  1. Developer is more likely to crash all application if not override well the methods

One note: If the commit messages are used for release notes. I guess that a have to rebase all this commits in one. The only problem is that I don't know if it is a fix or a feature anymore, mainly if we open space for new formats ;-)

giovannicandido avatar Sep 07 '14 00:09 giovannicandido

One quick comment before I can review this more deeply: I'm not sure if converter is good name (users could confuse it with Converter)

vicb avatar Sep 08 '14 07:09 vicb

Changed module name to object_parser Created abstracted class ObjectParser with methods encode, decode. Refactored JsonParser to implement ObjectParser as default implementation. This open space for XML and other formats.

I think that playback_http.dart and formatter/json.dart could use the JsonParser as well.

giovannicandido avatar Sep 26 '14 20:09 giovannicandido

@vicb let's discuss this week whether it makes sense to address this in 1.x.

naomiblack avatar Dec 09 '14 22:12 naomiblack