sequelize-to-json icon indicating copy to clipboard operation
sequelize-to-json copied to clipboard

Merging multiple schemes and one handy helper

Open Ajaxy opened this issue 7 years ago • 6 comments

Ajaxy avatar Feb 01 '17 00:02 Ajaxy

@hauru

Ajaxy avatar Feb 02 '17 00:02 Ajaxy

Hi, thanks for contributing and sorry to keep you waiting. I'm a bit busy at the moment but i'll try to review your commits today. Looks OK at first glance.

hauru avatar Feb 02 '17 10:02 hauru

Hey @Ajaxy. Sorry for the delay. I think the idea of having an ability to merge several serialization schemes certainly makes sense. However, after giving it all some thought, i'm seeing several problems with the way schemes are merged in your code. Sure, schemes can be combined generically - as objects - but the question remains will the resulting scheme indeed act as expected? Consider the following:

  1. Because the exclude list takes precedence over the include list, a field excluded in any of the merged schemes won't be included in the result no matter what if we use simple list concatenation. This can render certain groups of schemes useless when merged together. Better approach IMO would be to apply include / exclude lists consecutively for each scheme in the sequence.

  2. What if more than one scheme has the postSerialize hook defined? It might be good idea to chain postSerialize hooks into arrays and handle this case appropriately.

  3. Finally, my initial idea was to have all schemes in my code defined as explicitely as possible. Making it too easy to generate schemes ad-hoc (even by just merging other schemes) doesn't fully fit into the way i imagined it. On the other hand, being able to apply schemes to one another would definitely be useful. I think maybe for this purpose we could introduce new field(s) into Scheme object, like extendsScheme and/or includesSchemes. The first one would make the current scheme "inherit from" the given one, by taking the parent scheme's attributes and building over them. The second field would allow for inclusion of multiple named schemes into the current one. The order in which schemes are merged does actually matter, that's why two separate fields would be useful.

Have to meditate on this some more but i think such feature will come in few days.

hauru avatar Feb 05 '17 20:02 hauru

Hi. This makes sense. But it'll be a little harder to mix it the way serialize(user, ['default', 'withStatus', 'withParents', 'withChildren']). What if just to suggest a convention or disclaimer to not add anything ambiguous to mixing schemes?

Ajaxy avatar Feb 05 '17 23:02 Ajaxy

I can imagine that the main case of mixing is just adding some extras to a solid scheme.

Maybe add some partials and allow to mix only them, i.e. serialize(user, {extend: 'default', with: ['_status', '_parents', '_children']})

Ajaxy avatar Feb 06 '17 00:02 Ajaxy

Yeah, i think i might add a feature like this as well. Stay tuned.

hauru avatar Feb 07 '17 14:02 hauru