search

ignitionworks / draftjs_exporter

Export Draft.js content state into HTML.
MIT License
16 stars 20 forks source link

Ignore unknown entity type #6

Closed jibidus closed 6 years ago

jibidus commented 7 years ago

This commit may prevent KeyError exception (key not found: XXX) on unexpected entity types which is useful when you have no control on draftjs content.

threadhead commented 6 years ago

👍

theozaurus commented 6 years ago

I haven't merged this as throwing a key error was deliberate. I tend to find code that silently continues if something unknown is encountered is much harder to debug. Maybe the default behaviour could be set to throw an error, but allow some config to change that.

threadhead commented 6 years ago

This fork does not solve for all cases.

My issue with raising an error when an entity is not found is that it can be virtually impossible to handle all entity keys that may exist. I may be processing draftjs files from several sources for which I have no control. Like a dog chasing his tail.

May I suggest, like this PR attempts to solve, if there is no matching key then provide no element. Just assume it's either no-element text or

. I think that's an acceptable assumption.

I would support providing a datum of errors that occurred during translation. It's saying "hey, I converted your document to HTML but the following elements were not found so I just made them unstyled text."

theozaurus commented 6 years ago

The original idea for this library is to share the same (or as close to config) as the actual draftjs editor itself. In that way the same config controls the source and thus conversion in the browser, and the conversion on the server. With this setup there is never a missing key.

I'm not keen on a datum of errors. I can only really see two cases. Either you don't give a hoot about them (ignore the element), or you do - explode violently. A datum of errors allows for much more nuance but code that then reads that, and makes different decisions depending on the type of error seems too much.

I can see this is certainly an issue if you don't control the source. Why do you have no control over this?

threadhead commented 6 years ago

Editor(s) are on different services, of which we have no control.

theozaurus commented 6 years ago

@threadhead I'm happy to merge a pull request that has the default throw an error, but allows some configuration to change it - I think that will suit your usecase. Please make sure it's got tests and the config details in the README.

I'm going to close this particular pull request now.