Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrating react. #94

Closed
wants to merge 54 commits into from
Closed

Integrating react. #94

wants to merge 54 commits into from

Conversation

Gozala
Copy link

@Gozala Gozala commented Dec 18, 2014

This pull request is not ready to be merged yet, but I wanted to share it sooner to let you know I'm working on this & explain why. For one I believe use of react (or other similar style library) provide enough benefits to justify the switch. I'm going to list pros and cons as I see them below:

Pros

  1. Complex applications like browsers tend to be prone to certain style of issues. In my experience that usually caused by a multidirectional data flow and fragmented state scattered across the runtime. React embraces unidirectional data flow & single app level data storage that makes reasoning about code very trivial.
  2. In react synchronization of state with view (html) is taken care of by a library in very performant manner (redraws happen on animation frames) and application code is not constrained with manually updating dom instead it just expresses decleratively how state is mapped to the view. That makes code easier to reason & change as timing issues are really hard to run into as react uses virtual dom and performs complete DOM update in one transaction.
  3. Facebook is using react and putting a lot of effort into it, optimizing it for performance etc...
  4. React components tend to have a very clear interface and usually prevent you from coupling.
  5. Bunch of people know react already and arguably it's easier for ppl to contribute because it's opinionated enough to enforce certain style but not too much.

Cons

  1. React does not very friendly to non-standard html elements or attributes, there for I had to convert hbox, vbox, flex etc... to class names. I don't think that's a huge deal but definitely far from ideal. This limitation is little more annoying with web components, as custom event names aren't supported neither are attributes. That being there are lifecycle methods that let you make your own updates to DOM which is what I've used for Frame component. I imagine this limitation will be fixed over time, as I imagine that with a an adoption of web components this problem will be far more relevant.
  2. State updates happen should be handled by a root component, there for nested ones need to manually send requests. This is a cost one needs to pay in order to keep app state in one place, which results in little more boilerplate although on flip side having unified view of state has ton of benefits & it does make interface between components strictly defined.
  3. Unlike in current prototype different components do need to be wired, meaning that it's not enough to remove required module to enabling / disabling feature, although it's still easy enough to do. With little more effort I imagine it will be possible to reduce coupling even further. I think that can be done via something like an overlay component or alternatively by providing some API to registering components into a root component. I have not actually looked into this yet though.

@paulrouget
Copy link
Owner

This limitation is little more annoying with web components

Hmm, that's annoying. We really want to use web components. Any idea how to work around that?

State updates happen should be handled by a root component

I don't think this is a problem.

Unlike in current prototype different components do need to be wired, meaning that it's not enough to remove required module to enabling / disabling feature

Right. We can work around that.

@donaldpipowitch
Copy link
Contributor

Has anyone tried virtual-dom? Has it the same limitation? (It seems, no.) I avoided React so far, because it messes with Web Components.

@Gozala
Copy link
Author

Gozala commented Dec 19, 2014

This limitation is little more annoying with web components

Hmm, that's annoying. We really want to use web components. Any idea how to work around that?

Well so far I just implemented react component instead of Web Component, I don't think web component buys us that much at least at this point. You can actually define your own tags, but non "data-" attributes can't be extended as far as I can tell. I have submitted this issue facebook/react#2746 (comment)

That being said you can always wrap web component into react component and take care of all updates manually. It's not terrible but you end up throwing out a lot of benefits that reacts gives though.

@Gozala
Copy link
Author

Gozala commented Dec 19, 2014

Has anyone tried virtual-dom? Has it the same limitation? (It seems, no.)

Yes I've used vtree and vdom that are main building blocks for virtual-dom quite successfully.

I have building a todo-mvc that runs all of the app in a web worker. You can find code here https://github.com/Gozala/reflex/tree/workers/examples

I have also prototyped clone of react's API on top of vtree and vdom here
https://github.com/Gozala/addon-sdk/blob/react/lib/re-act/component.js

I do also want to prototype component API on top of vtree and vdom that defines web components under the hood.

That being said while I think vtree and vdom are good building blocks I'd rather start with react and then maybe swap it out something based off vtree / vdom.

I avoided React so far, because it messes with Web Components.

I don't think it messes with WebComponents it just does not supports them & they the main reason is they won't be able to perform all the optimizations they can currently do because of virtual dom.

@Gozala
Copy link
Author

Gozala commented Dec 29, 2014

Removed all the obsolete modules to make what's there more clear. Which made me realize I still need to port dark-theme and side tabs to use react components.

@paulrouget
Copy link
Owner

BTW could you point out inline bits that aren't obvious so I could improve by splitting expressions into multiple statements or providing comments.

I'll review these changes and let you know.

@Gozala
Copy link
Author

Gozala commented Dec 29, 2014

Removed all the obsolete modules to make what's there more clear. Which made me realize I still need to port:

  • dark-theme
  • side tabs

@@ -0,0 +1,79 @@
define((require, exports, module) => {
"use stict";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo here, r missing in strict.

@Gozala
Copy link
Author

Gozala commented Dec 30, 2014

Ok support for sidetabs is inn now as well. I have also removed all the unused code (with except of css to make it easy merge changes in master). Also source should be full of semicolons now as well.

@Gozala
Copy link
Author

Gozala commented Dec 30, 2014

P.S.: Diffstat says +19,653 −1,483 but that's misleading since react adds +18,095 is + so if we ignore that it would be more like +1,558 −1,483 & then if also remove legacy css I think we might come out even.

@Gozala
Copy link
Author

Gozala commented Jan 5, 2015

@paulrouget what should I do with this pull request ?

@Gozala
Copy link
Author

Gozala commented Jan 6, 2015

Moved this here browserhtml/browserhtml#34

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants