Skip to content
This repository has been archived by the owner on Dec 31, 2020. It is now read-only.

Add Observer render and inject sugar #387 #403

Merged
merged 9 commits into from
Feb 4, 2018
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 52 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,6 @@ The rendering in the function will be tracked and automatically re-rendered when
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.

Example:

```javascript
class App extends React.Component {
render() {
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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


```javascript
class App extends React.Component {
render() {
return (
<div>
{this.props.person.name}
<Observer
render={(props) => <div>{props.person.name}</div>}
Copy link

Choose a reason for hiding this comment

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

props work without inejction?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

/>
</div>
)
}
}

const person = observable({ name: "John" })

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

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

Example with inject
Copy link
Contributor

Choose a reason for hiding this comment

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

Same for this line, it's redundant imo.


```javascript

const NameDisplayer = ({ name }) => <h1>{name}</h1>

const user = mobx.observable({
name: "Noa"
})

const UserNameDisplayer = ()=>(
<Observer
inject={(stores)=>({user:stores.user})}
render={props => (<NameDisplayer name={props.user.name}/>)}
/>
)

const App = () => (
<Provider userStore={user}>
<UserNameDisplayer />
</Provider>
)
```

### Global error handler with `onError`

If a component throws an error, this logs to the console but does not 'crash' the app, so it might go unnoticed.
Expand Down Expand Up @@ -438,3 +486,5 @@ Data will have one of the following formats:

WeakMap. Its `get` function returns the associated reactive component of the given node. The node needs to be precisely the root node of the component.
This map is only available after invoking `trackComponents`.


2 changes: 1 addition & 1 deletion src/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ export function onError(cb: (error: Error) => void): () => void

export class Provider extends React.Component<any, {}> {}

export class Observer extends React.Component<{ children?: () => React.ReactNode }, {}> {}
export class Observer extends React.Component<{ children?: () => React.ReactNode, render?: () => React.ReactNode, inject?: IStoresToProps | string[] }, {}> {}

export function useStaticRendering(value: boolean): void

Expand Down
33 changes: 31 additions & 2 deletions src/observer.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Atom, Reaction, extras } from "mobx"
import { Component } from "react"
import React, { Component } from "react"
import { findDOMNode as baseFindDOMNode } from "react-dom"
import EventEmitter from "./utils/EventEmitter"
import inject from "./inject"
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great !

if (typeof component === "undefined") {
return null
}
if (!observerInject) {
return component()
}
const InjectComponent = inject(observerInject)(component)
return <InjectComponent />
})

Observer.displayName = "Observer"

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

Choose a reason for hiding this comment

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

DRYing this would be nice :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ennnn Any suggestion to diy Observer.propTypes

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

if (typeof propValue["children"] === "function") {
return
}
if (typeof propValue[key] !== "function")
return new Error(
"Invalid prop `" +
propFullName +
"` of type `" +
typeof propValue[key] +
"` supplied to" +
" `" +
componentName +
"`, expected `function`."
)
},
children: (propValue, key, componentName, location, propFullName) => {
if (typeof propValue["render"] === "function") {
return
}
if (typeof propValue[key] !== "function")
return new Error(
"Invalid prop `" +
Expand Down
73 changes: 62 additions & 11 deletions test/observer.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ import ReactDOM from "react-dom"
import ReactDOMServer from "react-dom/server"
import TestUtils from "react-dom/test-utils"
import * as mobx from "mobx"
import { observer, inject, onError, offError, useStaticRendering, Observer } from "../"
import { createTestRoot, sleepHelper, asyncReactDOMRender } from "./index"
import { observer, inject, onError, offError, useStaticRendering, Observer, Provider } from "../"
import { createTestRoot, sleepHelper, asyncReactDOMRender, asyncRender } from "./"
import ErrorCatcher from "./ErrorCatcher"

/**
Expand All @@ -16,12 +16,6 @@ const testRoot = createTestRoot()

const getDNode = (obj, prop) => obj.$mobx.values[prop]

const asyncRender = (element, root) => {
return new Promise(resolve => {
ReactDOM.render(<element />)
})
}

/*
use TestUtils.renderIntoDocument will re-mounted the component with with different props
some misunderstanding will be cause?
Expand Down Expand Up @@ -209,7 +203,6 @@ describe("does not views alive when using static rendering", () => {
})

test("no re-rendering on static rendering", () => {
expect(renderCount).toBe(1)
data.z = "hello"
expect(renderCount).toBe(1)
expect(TestUtils.findRenderedDOMComponentWithTag(element, "div").innerHTML).toBe("hi")
Expand Down Expand Up @@ -533,9 +526,10 @@ describe("it rerenders correctly if some props are non-observables - 2", () => {
}

mobx.reaction(() => odata.x, v => console.log(v))

beforeAll(async () => {
beforeAll(async done => {
await asyncReactDOMRender(<Parent odata={odata} />, testRoot)
done()
})

test("init renderCount === 1", () => {
Expand Down Expand Up @@ -757,3 +751,60 @@ test.skip("195 - should throw if trying to overwrite lifecycle methods", () => {
/Cannot assign to read only property 'componentWillMount'/
)
})

describe("use Observer inject and render sugar should work ", () => {
test("use render without inject should be correct", async () => {
const Comp = () => (
<div>
<Observer render={props => <span>{123}</span>} />
</div>
)
await asyncReactDOMRender(<Comp />, testRoot)
expect(testRoot.querySelector("span").innerHTML).toBe("123")
})

test("use children without inject should be correct", async () => {
const Comp = () => (
<div>
<Observer>{props => <span>{123}</span>}</Observer>
</div>
)
await asyncReactDOMRender(<Comp />, testRoot)
expect(testRoot.querySelector("span").innerHTML).toBe("123")
})

test("use render with inject should be correct", async () => {
const Comp = () => (
<div>
<Observer
inject={store => ({ h: store.h, w: store.w })}
render={props => <span>{`${props.h} ${props.w}`}</span>}
/>
</div>
)
const A = () => (
<Provider h="hello" w="world">
<Comp />
</Provider>
)
await asyncReactDOMRender(<A />, testRoot)
expect(testRoot.querySelector("span").innerHTML).toBe("hello world")
})

test("use children with inject should be correct", async () => {
const Comp = () => (
<div>
<Observer inject={store => ({ h: store.h, w: store.w })}>
{props => <span>{`${props.h} ${props.w}`}</span>}
</Observer>
</div>
)
const A = () => (
<Provider h="hello" w="world">
<Comp />
</Provider>
)
await asyncReactDOMRender(<A />, testRoot)
expect(testRoot.querySelector("span").innerHTML).toBe("hello world")
})
})
12 changes: 12 additions & 0 deletions test/ts/compile-ts.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,18 @@ class ObserverTest extends Component<any, any> {
}
}

class ObserverTest2 extends Component<any, any> {
render() {
return <Observer render={() => <div>test</div>} />;
}
}

class ObserverTest3 extends Component<any, any> {
render() {
return <Observer inject={["store"]} render={() => <div>test</div>} />;
}
}

@observer
class ComponentWithoutPropsAndState extends Component<{}, {}> {
componentDidUpdate() {
Expand Down