search

guardian / scribe

DEPRECATED: A rich text editor framework for the web platform
http://guardian.github.io/scribe/
Apache License 2.0
3.51k stars 245 forks source link

Incorporate ImmutableJS #295

Closed rrees closed 9 years ago

rrees commented 9 years ago

OMG! Totes immutes!

This introduces the ImmutableJS library, converts the formatters over to Immutable structures and as a consequence removes the LoDash flatten function.

I also tried to replace defaults with merged maps but there is some subtly in the logic that I am missing.

I also attach the Immutable library to the scribe object so that plugins can make use of it.

QA

This doesn't change any functionality, just the way it is implemented. In theory it is a genuine for-real refactoring with passing tests.

However the way the formatters get registered has changed so it might be worth checking the default behaviour of the standard plugins and the external plugins may need to be rechecked as they get deployed.

hmgibson23 commented 9 years ago

:+1: I'm happy with this but we should check the plugins before releasing.

theefer commented 9 years ago

I would like to point out that this more than doubles the size of Scribe.

Scribe is 38kb minimised, immutablejs is 55kb minimised.

Do we think we will get enough benefit to justify that bloat?

theefer commented 9 years ago

And btw I am normally the last person to put performance/optimisations before correctness (we don't even bundle, minimise or gzip assets in the Grid at the moment rofl!!1), but size is/was an asset of Scribe. I also write mostly immutable JS, so I'm all for that, but would be nice to briefly pause and think of where else we could apply this in the codebase?

vdom comes to mind, @robinedman would virtual-dom play nice with such externally-brought immutable library, or would the vdom AST itself need to be made of immutable objects?

robinedman commented 9 years ago

@theefer Will probably play nice since virtual-dom is immutable by convention (the README states it's for performance reasons).

I agree the size is a bit worrying.

rrees commented 9 years ago

For me 55K isn't something significant for our own tools (any raster image in the interface will be larger in terms of footprint) but if it makes the editor less attractive external users then maybe we can have a minimal Scribe build, in which case we should drop the mandatory patches and formatters from the main project.

hmgibson23 commented 9 years ago

Makes sense to have a minimal build I think. I'm kind of indifferent to size - although I shouldn't be!

rrees commented 9 years ago

I'm planning to merge this next week.

hmgibson23 commented 9 years ago

I'm happy with it; I guess only @theefer or can talk you out of it now!

On 5 December 2014 at 12:45, Robert Rees notifications@github.com wrote:

I'm planning to merge this next week.

— Reply to this email directly or view it on GitHub https://github.com/guardian/scribe/pull/295#issuecomment-65785163.

Visit theguardian.com. On your mobile and tablet, download the Guardian iPhone and Android apps theguardian.com/guardianapp and our tablet editions theguardian.com/editions. Save up to 57% by subscribing to the Guardian and Observer - choose the papers you want and get full digital access. Visit subscribe.theguardian.com

This e-mail and all attachments are confidential and may also be privileged. If you are not the named recipient, please notify the sender and delete the e-mail and all attachments immediately. Do not disclose the contents to another person. You may not use the information for any purpose, or store, or copy, it in any way. Guardian News & Media Limited is not liable for any computer viruses or other material transmitted with or as part of this e-mail. You should employ virus checking software.

Guardian News & Media Limited is a member of Guardian Media Group plc. Registered Office: PO Box 68164, Kings Place, 90 York Way, London, N1P 2AP. Registered in England Number 908396

hmgibson23 commented 9 years ago

I think we should have brief chat about it Monday, \cc @theefer @robinedman @cutandpastey

theefer commented 9 years ago

To me, it'd be interesting to demonstrate where the value would come, i.e. what parts of Scribe would benefit from doubling its size. The example in this PR are nice to have but really not enough to justify it imho.

Your call, but I personally just don't see the value yet, though I'd love to be shown.

rrees commented 9 years ago

@theefer For me the network bandwidth size issue is just a red herring, you're gaining determinism and structure sharing memory structures (reduced memory allocation in operation).

theefer commented 9 years ago

I completely agree with the rationale, I guess I'm just curious if and where such structures will be used in Scribe (besides the trivial example in this PR), which is unfortunately highly state full and mutation-based at the moment.

theefer commented 9 years ago

Immutable transformation of vdom trees come to mind, for instance, so it'd probably more convincing to bring in immutable as part of a proof that it can actually be used for vdom (I have some doubts given that virtual-dom trees' reliance on native data structures, such as array for children and objects for attribute maps, seems to be hardcoded in, but I may well be wrong).

philfreo commented 9 years ago

Loved that Scribe was so minimalistic. Seems strange to have gone from 39kb to 93kb without great reason.

chrismendis commented 9 years ago

Why weren't the command and commandPatches objects refactored in to Immutable Maps?

rrees commented 9 years ago

@chrismendis There wasn't a big reason, ultimately we want to do that. Pull request?