formly-js / angular-formly

JavaScript powered forms for AngularJS
http://docs.angular-formly.com
MIT License
2.22k stars 405 forks source link

Initially null vs empty object for field groups #614

Closed koraybalci closed 8 years ago

koraybalci commented 8 years ago

I have formly field with a fieldGroup inside (like a sub form of some sort). the field is tied to model via key and everything is playing almost nicely. However, the initial (untouched) value of the key object is not null but empty object {}, which is set by formly I presume.

Not sure if this is a bug or sth by design, but when things are nested and json is sent to server over the wire, those empty objects become empty entities on server side which can create all sorts of problems. I can manually go over the objects (recursively) in the model and nullify them but I suppose if they were initially null, that would be much cleaner.

I also can't set defaultValue to null for fields with fieldGroups, if that makes sense.

Not sure if what I am asking is even possible but can formly set these objects to null initially (instead of empty objects)?

I hope I am not missing sth.

benoror commented 8 years ago

Do you mind creating an example of what you're trying to accomplish? Please follow the instructions here: http://help.angular-formly.com

This will make it easier for you to get help. Because the github issues are reserved for bug reports and feature requests, I'm going to go ahead and close this issue. See you on chat! Thanks!

koraybalci commented 8 years ago

I am sorry but I believe this is either bug report or feature request if not both. I will try to provide you with a simple example, however I wish you have at least read it through before closing the issue.

benoror commented 8 years ago

Hi @koraybalci , The reason is that it is difficult for maintainers to keep up with issues. Having an example would help us quickly diagnose the problem, which most of the time are not bugs nor features requests, but simply misuse or lack of understanding of the library. Therefore the suggested channels (chat, stack overflow, mailing list) are much better suited for those cases.

Anyway, if you can make an example (new-example.angular-formly.com) to reproduce the issue I can give you some feedback right here :smile:

koraybalci commented 8 years ago

Thanks for the explanation, I totally understand the policy, I hope I wasn't rude or sth.

However, to reproduce the issue, just open the example http://angular-formly.com/#/example/other/nested-formly-forms and comment out the initialization of address field in vm.model such that:

vm.model = {      
     // firstName: 'Benjamin',
     // address: {
     //   town: 'San Luis Potosi',
     //  }    
};

and re-run the example and observe vm.model in output window. You will see that vm.model.address is initialized to empty object {}

My point/question is, shouldn't it be set to null instead of empty object. I think it's either a bug or wrong implementation because of the side effects I mention in my original post.

benoror commented 8 years ago

From my understanding a data model with N levels of empty nested models should look like this:

X= {
  Y: {
    A: {
      K: {}
    },
    B: {}
  }
}

Setting an empty model to null wouldn't make sense in this scenario, that's why formly defaults to an empty object {}.

Maybe the issue is the way it's handled in your backend, what server technology do you use?

koraybalci commented 8 years ago

Well, sorry but I still tend to disagree. I understand how a nested model should look like, but unless there is no actual data, each property can stay null as long as their values (or their children's values) remain untouched.

I am using a standard c# webapi backend with json.net serializer, which converts the mentioned empty js object to an empty (not null but instantiated) c# object. I don't think there is anything special or wrong with my backend, and philosophically (or architecturally speaking) the backend is irrelevant. I may be able to handle it in the backend but it would be patching the client behavior in the server side, which may miserably fail in the future.

To put it simply, in my understanding and from my perspective (be it client or server side does not matter, I am speculating sort of philosophically here), if there is nothing in the form, there should be no data, and no data means null, not empty object.

I don't know how this case is actually implemented in formly, but I would prefer a lazy initialization scheme if possible. That would be in line with the behavior common to any other model field (simple text input etc) in formly.

benoror commented 8 years ago

Fair enough, you have 2 options:

  1. Test, fix and submit a PR
  2. Transform your payload before sending it back to the backend. It would be trivial to use Underscore/Lodash for such task, a quick google search spit this out: http://stackoverflow.com/questions/14058193/remove-empty-properties-falsy-values-from-object-with-underscore-js
koraybalci commented 8 years ago

Thanks, I already took the 2nd route, however because of the recursive nature of the problem (nested stuff), I was hesitant on performance implications and I do believe that kind of patching should be unnecessary. Hence I opened an issue to understand whether it is a fix needed on lib level.

I can try to patch it as you suggest in your option 1, if we both (all) agree it's a lib level fix and no nasty side effects will arise. I am especially worried about other people's client code which rely on existing behavior. I'd appreciate if you can reopen the issue too, if that's the route you'd prefer.

By the way thanks for the discussion and fast response.

benoror commented 8 years ago

ping @kentcdodds (maybe Kent can provide valuable feedback :smile:)

kentcdodds commented 8 years ago

Thanks for the help @benoror.

Hmmm.... Yeah, I feel like this is a bit of an edge case and would prefer that it's handled outside of angular-formly unless I can see a simple, tested implementation. My inclination is to say this is outside of the scope of the project though. Sorry @koraybalci.

koraybalci commented 8 years ago

Fair enough (although I still tend to disagree :) ). I am closing the issue then.