search

Yomguithereal / react-blessed

A react renderer for blessed.
MIT License
4.45k stars 177 forks source link

Allow other blessed apps #79

Closed abritinthebay closed 6 years ago

abritinthebay commented 6 years ago

Given blessed is... somewhat unmaintained, it would be nice to be able to pass in a different function to be used (ie - only require blessed if none is passed). That way we could use forked versions with updated code.

Thoughts?

Yomguithereal commented 6 years ago

Hello @abritinthebay. Since currently react-blessed only requires blessed to do some argument validation here, I guess it wouldn't be very hard to drop it from the code and let the user install whichever version.

d4rky-pl commented 6 years ago

@Yomguithereal the blessed is also used here and here. If I understand correctly, the idea was that render acts the same way as ReactDOM.render and accepts the same arguments. How do you think passing the blessed instance should be handled then?

I couldn't think of any way that wouldn't break backwards-compatibility, something like:

const render = createBlessedRenderer(blessed)

// and now render(<Foo/>, bar)

This could be added next to the old render to allow both but if I understand node packaging correctly, it'd still require bundling blessed itself with react-blessed which is a non-ideal situation (correct me if I'm wrong, I've always precompiled my libraries with webpack/rollup)

d4rky-pl commented 6 years ago

Looked at the compiled code and I see require calls are untouched which means we could add createBlessedRenderer as extra thing and make the old render still work. Let me know if you're ok with this approach and I'll come up with a PR.

abritinthebay commented 6 years ago

Sounds fine to me! Can always look st conditional requires to reduce package size at a later date!

d4rky-pl commented 6 years ago

Can I get a maintainer blessing so I know it makes sense to spend time on this PR? :) cc @Yomguithereal

Yomguithereal commented 6 years ago

Go ahead @d4rky-pl. We agree that a factory here would only enable people to use forked versions of blessed, right? Creating the renderer factory is easy however, creating factory for the reconciler etc. might prove a bit tedious to implement but I don't remember the code base enough to say.

d4rky-pl commented 6 years ago

@Yomguithereal since reconciler is never exported outside (because it's an implementation detail), I just wanted to move it's initialization to the factory method and take advantage of javascript scoping.

I'll try to spend some time this weekend to deliver a PR (or at least POC) but to be honest I expect this to be mostly a bit of code shuffling and not a major rewrite.