Add Observer render and inject sugar #387 #403

Sunshine168 · 2018-01-30T06:07:35Z

No description provided.

danielkcz · 2018-01-30T20:01:39Z

Just a hint, it might be easier to review this if you turn off Prettier :) (or perhaps eslint autofix?). Any kind of reformatting should be done in a separate PR in my opinion.

And I love this, that's mostly what I suggested in #387

Edit: Oh and would you mind updating Typescript definitions as well?

Sunshine168 · 2018-01-31T01:02:01Z

@FredyC
Oh i got a hobby which formate code anytime. i check package seem husky not be added, so precommit did not work well.
i thought the rep should be format by same coding style and precommit should work, following this the code will be easier to review

hhhh, i very appreciate your suggestion ,so i did it and will update typescript definitions later

danielkcz · 2018-01-31T10:01:23Z

Yea, you are right, husky is not installed so precommit hook is not working. I've made another PR #407 to fix that. Once that gets merged, you can simply rebase this (some conflicts might happen) and only relevant changes will be part of this PR.

mweststrate · 2018-02-01T08:59:56Z

@Sunshine168 Thanks for the PR! @FredyC's PR is merged, could you rebase?

PR looks good :). One remaining question; could you update the Readme as well describing this alternative syntax?

danielkcz · 2018-02-01T09:07:03Z

Also, it's obvious that using children has a priority, but there should be test showing a use of both at once. And reflect this in docs as well.

Sunshine168 · 2018-02-01T10:15:32Z

i will rebase later , i am still in working

Sunshine168 · 2018-02-01T13:20:52Z

done !

danielkcz · 2018-02-01T13:31:31Z

README.md

@@ -82,7 +82,7 @@ It takes as children a single, argumentless function which should return exactly
 The rendering in the function will be tracked and automatically re-rendered when needed.
 This can come in handy when needing to pass render function to external components (for example the React Native listview), or if you
 dislike the `observer` decorator / function.
-
+And it can work with `Provider` use inject  and render property / or just takes as children, detail in Example2.(PS:using children has a priority than render , so dont use at the same time)


I would split this, there are two concerns to cover - render prop and injecting. Also, I have a trouble understanding what it says. Let me rephrase this.

In case you are a fan of render props, you can use that instead of children. Be advised, that you cannot use both approaches at once, children have a precedence.

Here goes example showing render prop only

Observer can also inject the stores simply by passing a selector function.

Second example with inject

Oh and with injecting it's worth to show an example that passing strings work same way as with inject HoC.

danielkcz · 2018-02-01T13:32:58Z

README.md

+class App extends React.Component {
+  render() {
+     return (
+          <Provider h="hello" w="world">


Are you sure you can pass a string as a store prop? I am not aware that it would turn to observable automagically here. In that case, it's kinda misleading example.

danielkcz · 2018-02-01T13:35:08Z

src/observer.js


 Observer.displayName = "Observer"

 Observer.propTypes = {
+    render: (propValue, key, componentName, location, propFullName) => {


DRYing this would be nice :)

ennnn Any suggestion to diy Observer.propTypes

Well, if you look closely, it's the same function except for that first condition. Which doesn't make any sense to me. It means, that when checking render prop, you are telling it's ok if children prop is a function. That doesn't make any sense to me.

danielkcz · 2018-02-01T13:37:20Z

README.md

@@ -82,7 +82,7 @@ It takes as children a single, argumentless function which should return exactly
 The rendering in the function will be tracked and automatically re-rendered when needed.
 This can come in handy when needing to pass render function to external components (for example the React Native listview), or if you
 dislike the `observer` decorator / function.
-
+And it can work with `Provider` use inject  and render property / or just takes as children, detail in Example2.(PS:using children has a priority than render , so dont use at the same time)


Oh and with injecting it's worth to show an example that passing strings work same way as with inject HoC.

Sunshine168 · 2018-02-01T15:02:26Z

@FredyC Ohhhh very thank you for your help .tat now will be better?

danielkcz · 2018-02-01T18:53:35Z

README.md

 const Comp = () => (
    <div>
        <Observer
-            inject={store => ({ h: store.h, w: store.w })}
-            render={props => <span>{`${props.h} ${props.w}`}</span>}
+            render={() => <span>hello world</span>}


Hm, not very good example since there is nothing to observe :D Perhaps pass some variable in props to Comp and use that in render so it makes more sense.

danielkcz · 2018-02-01T18:54:06Z

README.md

-Example2:
+In case you are a fan of render props, you can use that instead of children. Be advised, that you cannot use both approaches at once, children have a precedence.
+
+Here goes example showing render prop only


Well, I did not mean this line to be included there as well, that's why I used italic ;)

danielkcz · 2018-02-01T19:02:45Z

src/observer.js


 Observer.displayName = "Observer"

 Observer.propTypes = {
+    render: (propValue, key, componentName, location, propFullName) => {


Well, if you look closely, it's the same function except for that first condition. Which doesn't make any sense to me. It means, that when checking render prop, you are telling it's ok if children prop is a function. That doesn't make any sense to me.

danielkcz

We are getting there, some small tweaks are remaining.

danielkcz · 2018-02-01T19:05:38Z

README.md

@@ -105,6 +103,56 @@ React.render(<App person={person} />, document.body)
 person.name = "Mike" // will cause the Observer region to re-render
 ```

+In case you are a fan of render props, you can use that instead of children. Be advised, that you cannot use both approaches at once, children have a precedence.
+
+Here goes example showing render prop only


This line is a bogus one, I did not mean to include it by using italic text :)

danielkcz · 2018-02-01T19:06:36Z

README.md

+
+Observer can also inject the stores simply by passing a selector function.
+
+Example with inject


Same for this line, it's redundant imo.

danielkcz · 2018-02-01T19:08:31Z

src/observer.js

@@ -349,12 +349,41 @@ function mixinLifecycleEvents(target) {
 }

 // TODO: support injection somehow as well?
-export const Observer = observer(({ children }) => children())
+export const Observer = observer(({ children, inject: observerInject, render }) => {
+    const component = children || render


I am thinking if there shouldn't be some warning if both props are used. Perhaps that's what you had in mind with that propTypes check? Except you need to return message describing a problem. Returning undefined means it's valid.

ennn , thinging, first i prefer show warning by propTypes if both props
and coding like this

const ObserverPropsCheck = (props,key,componentName,location,propFullName)=>{ const extraKey = key === "children" ? "render" : "children" if(typeof propValue[key] === "function" && typeof propValue[extraKey] === "function" ){ return new Error("Invalid prop,do not use children and render in the same time in`" + componentName ) } if (typeof propValue[key] === "function" || typeof propValue[extraKey] === "function") { return } return new Error( "Invalid prop `" + propFullName + "` of type `" + typeof propValue[key] + "` supplied to" + " `" + componentName + "`, expected `function`." ) }

'>< but seem complexity
or do same props check in Observer Component instead in propType

Yea, checking with prop types like this is probably enough. I don't think it adds complexity, it's a friendly behavior toward user.

danielkcz

@mweststrate I think we are good to go here :)

Gvozd · 2018-02-03T20:10:49Z

README.md

+         <div>
+            {this.props.person.name}
+            <Observer
+              render={(props) => <div>{props.person.name}</div>}


props work without inejction?

Good catch, this is an oversight. It should refer to this.props instead.

Sorry ,a stupid fault zzzzzz should refer to this.props

mweststrate · 2018-02-04T15:12:08Z

Great work @Sunshine168 and thanks for the extensive reviews @FredyC and @Gvozd ! Will release tomorrow

mvestergaard · 2018-02-05T15:26:07Z

This is broken. Results in Warning: Failed prop type: propValue is not defined errors due to usage here https://github.com/mobxjs/mobx-react/blob/master/src/observer.js#L368

danielkcz · 2018-02-05T15:35:08Z

@mvestergaard That's an unfortunate oversight. Could you send in the PR please? Some tests for this should be probably included as well.

mweststrate · 2018-02-05T15:44:00Z

Just released 4.4.1, that should fix the issue. Unclear yet why this passed the tests, so if somebody could add a test testing the prop types checking that would be appreciated :D

danielkcz mentioned this pull request Jan 31, 2018

Add husky and run Prettier manually #407

Merged

mweststrate mentioned this pull request Feb 1, 2018

Update Observer component definition #376

Merged

Sunshine168 added 4 commits February 1, 2018 21:10

Add Observer render and inject sugar mobxjs#387

9a92db4

Update observer test

1edd04a

Add typescript definitions

1c54100

Add Observer render and inject sugar usage in README

13b416d

Sunshine168 force-pushed the observer-with-inject branch from 4c0173b to 13b416d Compare February 1, 2018 13:19

danielkcz reviewed Feb 1, 2018

View reviewed changes

danielkcz suggested changes Feb 1, 2018

View reviewed changes

Sunshine168 added 2 commits February 1, 2018 22:44

Update README

827a054

Update example

fac7e87

danielkcz reviewed Feb 1, 2018

View reviewed changes

danielkcz suggested changes Feb 1, 2018

View reviewed changes

Sunshine168 added 2 commits February 2, 2018 09:11

Remove bogus lines

f8c2cfb

Update Observer Props types Check

484aec2

danielkcz approved these changes Feb 2, 2018

View reviewed changes

Gvozd reviewed Feb 3, 2018

View reviewed changes

Fix Observer example

d4596da

danielkcz approved these changes Feb 4, 2018

View reviewed changes

mweststrate merged commit d921747 into mobxjs:master Feb 4, 2018

github-actions bot mentioned this pull request Oct 13, 2020

Next release #909

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Observer render and inject sugar #387 #403

Add Observer render and inject sugar #387 #403

Sunshine168 commented Jan 30, 2018

danielkcz commented Jan 30, 2018 •

edited

Loading

Sunshine168 commented Jan 31, 2018 •

edited

Loading

danielkcz commented Jan 31, 2018 •

edited

Loading

mweststrate commented Feb 1, 2018

danielkcz commented Feb 1, 2018

Sunshine168 commented Feb 1, 2018

Sunshine168 commented Feb 1, 2018

danielkcz Feb 1, 2018

danielkcz Feb 1, 2018

danielkcz Feb 1, 2018

danielkcz Feb 1, 2018

Sunshine168 Feb 1, 2018

danielkcz Feb 1, 2018

danielkcz Feb 1, 2018

Sunshine168 commented Feb 1, 2018

danielkcz Feb 1, 2018

danielkcz Feb 1, 2018

danielkcz Feb 1, 2018

danielkcz left a comment

danielkcz Feb 1, 2018

danielkcz Feb 1, 2018

danielkcz Feb 1, 2018

Sunshine168 Feb 2, 2018

danielkcz Feb 2, 2018

Sunshine168 Feb 2, 2018

danielkcz left a comment

Gvozd Feb 3, 2018

danielkcz Feb 3, 2018

Sunshine168 Feb 4, 2018

mweststrate commented Feb 4, 2018

mvestergaard commented Feb 5, 2018

danielkcz commented Feb 5, 2018

mweststrate commented Feb 5, 2018


		Observer can also inject the stores simply by passing a selector function.

		Example with inject

Add Observer render and inject sugar #387 #403

Add Observer render and inject sugar #387 #403

Conversation

Sunshine168 commented Jan 30, 2018

danielkcz commented Jan 30, 2018 • edited Loading

Sunshine168 commented Jan 31, 2018 • edited Loading

danielkcz commented Jan 31, 2018 • edited Loading

mweststrate commented Feb 1, 2018

danielkcz commented Feb 1, 2018

Sunshine168 commented Feb 1, 2018

Sunshine168 commented Feb 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sunshine168 commented Feb 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielkcz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielkcz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mweststrate commented Feb 4, 2018

mvestergaard commented Feb 5, 2018

danielkcz commented Feb 5, 2018

mweststrate commented Feb 5, 2018

danielkcz commented Jan 30, 2018 •

edited

Loading

Sunshine168 commented Jan 31, 2018 •

edited

Loading

danielkcz commented Jan 31, 2018 •

edited

Loading