sylvainpolletvillard / ObjectModel

Strong Dynamically Typed Object Modeling for JavaScript
http://objectmodel.js.org
MIT License
467 stars 30 forks source link

Is it possible to drop all keys with value undefined from model instance? #57

Closed serhiipalash closed 6 years ago

serhiipalash commented 6 years ago

Hi!

You have a grate library! It is the best thing I've worked with on modeling in JavaScript.

One question, is it necessary to add all unspecified model properties as undefined when creating the model instance?

Is there .toObject() method in the model?

sylvainpolletvillard commented 6 years ago

Hello, thanks !

If you have some properties in your model definition that are not assigned when creating the model instance, then these properties should be declared as optional properties

If you have some properties that are not in your model definition, it is also valid unless your model is sealed.

Can you explain what behaviour are you looking for with this toObject() method ? A code example could help me understand your question better.

serhiipalash commented 6 years ago

I run

const User = new ObjectModel({
  id: String,
  email: String,
  authProviders: AuthProviders,
  name: [String],
  birthday: [ISODate],
  language: [String],
  facebookId: [String],
})

const user = new User({
  id: 'john',
  email: 'test@test.com',
  authProviders: {
    facebook: true,
  },
})

console.info(user)

result is

screen shot 2017-11-08 at 01 31 42

I want result to be

screen shot 2017-11-08 at 01 32 41

Is it possible to avoid undefined in user getter?

serhiipalash commented 6 years ago

schema-object library has toObject() method for that https://github.com/scotthovestadt/schema-object#toobject

But it will be great to have similar functionality in your library as default behaviour. Without undefined it works like this.

serhiipalash commented 6 years ago

I want to run something like

saveToDatabase('/users', user)

It works now, but only if I provide all user properties.

Lifehack for any cases is

saveToDatabase('/users', JSON.parse(JSON.stringify(user)))
sylvainpolletvillard commented 6 years ago

Oh, don't worry about that, these undefined properties are not actually set. You can check by yourself:

user.hasOwnProperty("name") // should be false

and

JSON.stringify(user)
// "{"id":"john","email":"test@test.com","authProviders":{"facebook":true}}"

What happens is that when enumerating the properties of a model instance, the optional properties are counted even if they are not assigned yet. Also, the private properties are never shown in the enumerated properties.

This is done by using the ownKeys trap of the Proxy : https://github.com/sylvainpolletvillard/ObjectModel/blob/master/src/object-model.js#L148

This is intended to prevent developer mistakes when dealing with Object.keys / for..in loops on model instances with potential optional properties. This should not be a problem, but I understand it can cause confusion when looking at the string representation in the console.

serhiipalash commented 6 years ago

I want to use your library with Firebase and it requires simple objects. Can you add toObject() method?

firebase.database().ref('/users').set(user.toObject())
sylvainpolletvillard commented 6 years ago

I don't understand what this method toObject should do, and what is a "simple object" according to you.

The undefined optional properties will not be found in the output when serialized, as shown in my previous comment. So there is no need to "drop them" since they are not assigned in the first place.

I don't know the requirements of Firebase, and why it does not work directly with model instances. Do you have an error message or something ?

serhiipalash commented 6 years ago

I don't know how Firebase works, but when I pass model in it, it perceives data as

screen shot 2017-11-08 at 01 31 42

and raises an error because undefined not specified as allowed value in my database rules.

sylvainpolletvillard commented 6 years ago

I see. Firebase API must use Object.keys() internally and doesn't check if the keys are actually own properties.

If this is something required for external tooling integration, I'm going to change the ownKeys() behaviour to match the hasOwnProperty() behaviour. That is : 1) enumerable keys must be assigned to the instance 2) enumerable keys must be defined in the model definition 3) enumerable keys must not be private

The new rule here is the 1) ; it will solve your problem, but you will still have an error if you manually assign a property to undefined. I think it is the appropriate behaviour to expect here.

You still need to be aware of rules 2) and 3) to avoid any surprise if some props are missing on Firebase. Rule 3) can be avoided by overriding conventionForPrivate, and rule 2) is not a problem if you declare all the props in your definition (which is something you should do in general)

This is a breaking change so I'll push a new major version soon.

serhiipalash commented 6 years ago

Great! Thanks a lot!

These 3 rules are reasonable and I will follow them as well.

Your library is fully compatible with Firebase, only this one thing was missing.

sylvainpolletvillard commented 6 years ago

ObjectModel 3.2 has been released, can you try it and tell me if your undefined problems are gone ?

serhiipalash commented 6 years ago

One minute. I will try it.

serhiipalash commented 6 years ago

.extend() not working any more

I run

screen shot 2017-11-08 at 17 25 11

I see this

screen shot 2017-11-08 at 17 21 49

With previous version there were createdAt and updatedAt default values.

Here is my User and Timestamped models

screen shot 2017-11-08 at 17 20 49 screen shot 2017-11-08 at 17 21 24
serhiipalash commented 6 years ago

this is something else. I'm on it

Ok. Thanks!

sylvainpolletvillard commented 6 years ago

My investigation so far:

Object.keys() and Object.getOwnPropertyNames() return only the own properties of an object, not the properties that are declared in their prototype chain. And default properties are set in the model prototype, so technically they are not own properties.

That means the v3.2 behaviour is closer to the standard specification, but Firebase cannot work with objects with inherited properties, so your default properties are not passed.

Now, the question is: is it the right thing to do or not ?

The default values are applied when a value is missing, so there is no need to store the default values in the database ; when you will retrieve your objects from the database, the default values will be applied again. It also means you can update your default values and all the previously created instances will have their default values updated accordingly.

So I think the current behaviour makes sense, but it may not be what you want. If you think of "default value" as "initiate this prop with this value when it is undefined", as a one-time thing during instanciation, then defaults() is not the way to go. I would do it this way instead :

User.create = function(props){
  return new User(Object.assign({
     // assign defaults as own properties at instanciation
     lastLogin: ServerTimestamp  
  }, props))
}

const user = User.create({ email: "test@test.com" });

So there are two ways of thinking about default values, and both have valid usecases. If Firebase can handle optional properties, consider not storing default values to save space. If you want to always store all the values, default or not, let me know if the solution posted above works for you.

serhiipalash commented 6 years ago

let me know if the solution posted above works for you.

Ok, I will.

serhiipalash commented 6 years ago

Hi @sylvainpolletvillard !

You were right about default properties. The right way is v3.2 behaviour and when you want to create new instance with default props you need to do it with .create() method.

You can close this issue.

But I also wanted to ask you about using your library with Redux. How heavy are your models? I want to try to save User model instance as user object in Redux state. Has anyone done something like this?

sylvainpolletvillard commented 6 years ago

Hmm I never used Redux so I can't tell. But I try to make ObjectModel as "transparent" as possible so that you can use it with any other tool. That's why I made the change for Firebase.

So the best you can do is try out and give me feedback if something goes wrong. I am pretty confident it will be just fine.

I close the issue since the original issue is solved, but feel free to continue to chat here or on the gitter channel

serhiipalash commented 6 years ago

Ok. Thanks for your help!