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

Documentation improvements and questions #95

Open
holyjak opened this issue Mar 28, 2015 · 5 comments
Open

Documentation improvements and questions #95

holyjak opened this issue Mar 28, 2015 · 5 comments

Comments

@holyjak
Copy link

holyjak commented Mar 28, 2015

Hei, I have some proposals/questions bt could not find any e-mail group or similar to discuss them first so I am just creating an issue.

1. Differences between https://github.com/omniscientjs/omniscient and http://omniscientjs.github.io/

Currently the README at github contains some stuff not on the web page, e.g. the Local State section. That makes it harder to find stuff, it would be better if they were in sync.

2. Local State (vs. props)

  1. this.setState must be called from a mixin, not diectly from the component's (implicit) render method, right?
  2. Is this.state available inside the render method? It seems that when I use function as in component([withInitialState], function(props) { ...} then I can access this.state but if I instead use ES6 lambda as in: component([withInitialState], (props) => { ...} then this.state is undefined. This discrepancy should be documented or preferably prevented. Or is the plan that the state should be read/written only in a mixin? I.e. I'd need to add a getState to my mixin to access the state?
  3. You use usually the swapProps mixin instead of local state, right? It would be nice to explain why that is better in your view.

Multiple cursors

I believe I have seen in an example a component that depends on multiple cursors, something like

 MyComponent({cursor1: data.cursor("wines"), cursor2: data.cursor("desserts")})

Is that right? It would be nice to mention this possibility (and use case for it / how not to misues it) on the main page. It is obvious to you but to a newcomer like me, based on the examples, it seems that only a single cursor is typically passed in.

Limitations / when not to use?

Nothing is a silver bullet. It would be very useful if you described where you think Omniscient is a good fit and especially where it is not a good fit - to help me choose the right one of the many different React libraries. It would be helpful to list also the limitations / weaknesses of this approach, there are certainly some? I remember David Nolen describing issues with Om and some of that perhaps applies here as well (sadly, I do not remember the details and haven't the link; some of it was related to the inherently tree structure of the data, I believe).

Thank you!

PS: You can reach me via jakub (at) iterate.no if you want

@adamschoenemann
Copy link
Contributor

Regarding this.state in render function, your problem is probably related to function binding. When you use "lambda" style functions with e.g. React's preprocessor, the function is automatically bound to the the current this.

component((props) => {
  ...
});
// will be transformed to
component((function (props) {
  ...
}).bind(this));

That is why none of the component members are available when you use that style, because it is most likely bound to the window context.

@holyjak
Copy link
Author

holyjak commented Mar 30, 2015

Hi, thank you, Adam! Anyway it would be nice to stress this in the documentation. If "this" is not available, what workaround to use to access state/mixin methods?

@adamschoenemann
Copy link
Contributor

"this" will be automatically bound to the React component as long as you don't use lambda syntax for your render function :)

component((props) => {
    console.log(this); // => most likely "window"
    console.log(this.state); // undefined
});

component(function (props) {
   console.log(this); // => ReactComponent
   console.log(this.state); // Your component's state
});

@mikaelbr
Copy link
Member

Hi, @jakubholynet and thanks for your feedback!

  1. Differences between https://github.com/omniscientjs/omniscient and http://omniscientjs.github.io/
    Currently the README at github contains some stuff not on the web page, e.g. the Local State section. That makes it harder to find stuff, it would be better if they were in sync.

The README is ment as a summary / quick guide to what you need, and the homepage is more in-depth. I agree that some of the info on the README should also be mentioned on the homepage.


Local State (vs. props)

  1. this.setState must be called from a mixin, not diectly from the component's (implicit) render method, right?
  2. Is this.state available inside the render method? It seems that when I use function as in component([withInitialState], function(props) { ...} then I can access this.state but if I instead use ES6 lambda as in: component([withInitialState], (props) => { ...} then this.state is undefined. This discrepancy should be documented or preferably prevented. Or is the plan that the state should be read/written only in a mixin? I.e. I'd need to add a getState to my mixin to access the state?

As @adamschoenemann is pointing out here, this is to do with ES2015, and is maybe why a lot of people disagree with the syntactic sugar that is fat arrows. I don't feel Omniscient.js' documentation should be so verbose as to document JavaScript features/gotchas. In most cases, you can use fat-arrows as syntactic sugar to components as in most cases this won't be used. The easiest "solution" here, is to avoid the sugar, if it feels like "magic" and hides true intent.

var MyComponent = component(function (props) {
   return <div>{this.state.message}</div>;
});

Regarding this.setState in general, Omniscient.js tries to encourage not to use local component state. This is to not build up local, in-accessible state, within each component, but instead get passed the current values from the top. This way you get a referentially transparent system which is more predictable in it's output. Read more about the though behind this here: http://omniscientjs.github.io/guides/01-simpler-ui-reasoning-with-unidirectional/


Multiple cursors

I believe I have seen in an example a component that depends on multiple cursors, something like

 MyComponent({cursor1: data.cursor("wines"), cursor2: data.cursor("desserts")})

Is that right? It would be nice to mention this possibility (and use case for it / how not to misues it) on the main page. It is obvious to you but to a newcomer like me, based on the examples, it seems that only a single cursor is typically passed in.

Essentially, you can pass in any object literal as props, with support for cursors and immutable data. For instance, this can be done:

React.render(MyComp({
  cursor1: someCursor,
  cursor2: someOtherCursor,
  otherProperty: {
     myImmutableData: Immutable.fromJS({ hello: 'World' })
  }
}), document.body);

But should this be done? Probably not. As with normal functions, you wouldn't your parameter set to be too large, right? Something like function (a, b) { } might be a good function signature, but with a parameter list as function (a, b, c, d, e, f) { } the function is probably not "clean", and as too much responsibilities. In designing your application you might want to think of the "data contracts" you pass down to your components, and components should have single responsibilities. In some cases though, pragmatism comes and requires multiple cursors as inputs to a component, and in those cases the Omniscient components support that.


Limitations / when not to use?

Nothing is a silver bullet. It would be very useful if you described where you think Omniscient is a good fit and especially where it is not a good fit - to help me choose the right one of the many different React libraries. It would be helpful to list also the limitations / weaknesses of this approach, there are certainly some? I remember David Nolen describing issues with Om and some of that perhaps applies here as well (sadly, I do not remember the details and haven't the link; some of it was related to the inherently tree structure of the data, I believe).

As with all tools, this is in no way a silver bullet. Not every application is the best way to model in this way. For instance, the Atom.io editor just transformed to not use Reacts re-render model as it caused too many repaints. The same drawbacks of React will also, most likely, be apparent with Omniscient.js - all though, Omniscient would in most cases reduce some GC work and the render functions will be invoked less. That is another point, when the re-render interval is very high, and the virtual DOM is created many times, the GC might have a bad time clearing all of the objects created, thus the memory and CPU usage could get high.


Thanks for your thoughts. I hope my comments help some. I've also updated #56 to have check points for your feedback.

@holyjak
Copy link
Author

holyjak commented Mar 30, 2015

Thanks!

Ad this) Right, Omniscient shouldn't document JS. But since most examples use => it would be nice to provide an example that needs to access this (f.ex. to pass a callback from a mixing to an onClick) and explicitely mention that we have to use function to have access to this = the component.

Ad local state) I have read the essey and understand the reasons. Yet there are cases when state is valuable. YMMV :)

Ad multiple cursors) Yes, everything can be overused. Still I think it would be valuable to mention this, even if passing multiple cursors is normally not encouraged. But you always encounter special cases ...

Ad limitations) I was actually asking about the specific limiations/prefered use cases of Omniscient wrt. other React libraries. Or is Omnicient the absolutely best React library for any case? But I can understand if you do not have yet enough field reports to answer that well.

Thank you!

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

No branches or pull requests

3 participants