ianstormtaylor / slate

A completely customizable framework for building rich text editors. (Currently in beta.)
http://slatejs.org
MIT License
29.71k stars 3.24k forks source link

remove "terse" representation in Raw serializer #585

Closed ianstormtaylor closed 7 years ago

ianstormtaylor commented 7 years ago

I feel like it adds extra complexity to dealing with "raw" that isn't really needed for most cases, and if it really is needed for someone, they're probably going to want to go with an even more minified representation that saves more bytes with keys being minified and other things too.

Right now I think it's really only useful to make typing JSON representations slightly easier, which isn't enough gain for the uncertainty it introduces.

@SamyPesse @Soreine curious if you can think of any other "legit" uses of terse mode that I haven't thought of, or whether you've also avoided it.

oyeanuj commented 7 years ago

@ianstormtaylor the premise for terse was that it saves space by only representing what is needed later for deserialization. if you remove terse, it might be helpful to provide a guide/example of what is needed/not-needed for someone to implement their 'own' implementation.

personally, i think its valuable to save only what's needed and nothing more even if you don't minify the representation in any way. it seems cleaner to be able to save, retrieve and insert the state back without any manipulation of any sort.

ianstormtaylor commented 7 years ago

@oyeanuj thanks for the input!

I think potentially what I've discovered is that the "terse" representation should be separate from the idea of "serializing to a raw JSON representation".

The main issue I've found is that the terse representation is not friendly to running migrations, or doing any traversal/manipulation of the JSON, since you have to constantly guard against certain features either being there or not being there (depending on how much it was able to be "minified"). Which means for an logic where you actually want to do stuff with the raw representation without having to serialize it to the immutable one, you don't really want terse.

And trying to detangle that complexity is more difficult since the Html serializer actually supports serializing to a "terse" Raw object as well. It would be better I think if "terse" was removed completely from Slate's core, and treated as a third-party modification that is purely database specific for storage.

oyeanuj commented 7 years ago

@ianstormtaylor thanks for the detailed explanation! all of that makes sense. it would be very helpful to have a guide or example, once removed from core, of the recommended terse strategy for database storage.

thesunny commented 7 years ago

That makes sense to me as well Ian. I think anything that reduces the complexity (and the number of edge cases to have to be dealt with) is a good thing.

I did, however, want to add that the terse mode (at least for me) has been valuable when populating fake data into the editor while testing. It's not the end of the world to have to add the extra empty key/value pairs in, but thought I'd mention it.

One option may be to have to include the terse deserializer in a deeper part of the path. For example, by using import terseDeserializer from 'slate/terse-deserializer'. If done this way, it shouldn't add any code bloat unless it is explicitly required in.

That said, I think it's perfectly okay to remove it from core completely as well (although I'd certainly make it available as a separate package since I'm sure lots of code depends on terse support).

Sunny

thepian commented 7 years ago

I wouldn't want to rely on it if it isn't in the slatejs package. Then I risk that it has bugs and my data is screwed. I was planning to persist using terse, as I haven't seen any need for the full graph. From this I gather that terse isn't a good persistence format, in which case I don't get the point of terse. It seems that it's a pretty print function.