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

Idea: make inject a reactive function as well #111

Closed
mweststrate opened this issue Aug 25, 2016 · 9 comments
Closed

Idea: make inject a reactive function as well #111

mweststrate opened this issue Aug 25, 2016 · 9 comments

Comments

@mweststrate
Copy link
Member

See this SO issue: http://stackoverflow.com/questions/39128210/mobx-react-inject-and-observe-together

This means that any props preprocessing done by inject could be tracked as well, if the generated injectWrapper component would be an observer itself as well

@jrmyio
Copy link

jrmyio commented Oct 7, 2016

This would be a great feature as it allows you to make components that are not strictly bound to a mobx store as you will be able to feed the components with only the stuff it needs directly like one does in redux mapToProps/dispatchToProps.

@mweststrate mweststrate mentioned this issue Nov 7, 2016
13 tasks
@Strate
Copy link
Contributor

Strate commented Nov 8, 2016

I think that it could be considered as a bug. This is really annoying to developer, when reactive library became non-reactive (does not trigger re-render on some cases). I think we should reduce that cases as much as possible.

@mweststrate
Copy link
Member Author

mweststrate commented Nov 9, 2016

@ConneXNL / @Strate I think it would allow for things like:

const App = inject(stores => ({
  name: stores.user.name
}, ({name}) => <MyNonReactiveComponentFromExternalLib name={name} />)

But, then

const App = observer(["user"], ({user} => <MyNonReactiveComponentFromExternalLib name={user.name} />))

Might just be more straight forward?

On the other hand, it might be confusing for people, especially coming from Redux, that injecting like below doesn't work (changing the counterStore does not re-render Counter, because inject itself is not observer:

import React from 'react';
import { connect } from 'utils';

const selector = (stores) => {
  return {
    counter: stores.counterStore.counter,
    double: stores.counterStore.double,
    increment: stores.counterStore.increment
  };
};

const Counter = ({ counter, double, increment }) => {
  return (
    <div className="counter">
      <p>{ counter }</p>
      <p className="double">{ double }</p>
      <button onClick={increment}>+1</button>
    </div>
  );
};

export default inject(selector, observer(Counter));

(see: http://stackoverflow.com/questions/39128210/mobx-react-inject-and-observe-together)

@mweststrate
Copy link
Member Author

@ConneXNL you can try the reactive inject with build [email protected]. The signature of the mapper function (stores, props, context) => newProps is unchanged

@jrmyio
Copy link

jrmyio commented Nov 14, 2016

As discussed on gitter. The following problem occurs that incorrectly re-renders observable components:
https://gist.github.com/ConneXNL/32f65c708efb61af3e3e2d6f2df4aa4c

highlighted: expr(() => state.isHighlighted(item)) fixes the problem but shouldn't be required.

@jrmyio
Copy link

jrmyio commented Nov 17, 2016

Awesome to see this one getting into mobx!

The above problem is fixed with #160 .

However, the mobxjs/mobx-react-devtools does log 'inject-MyComponent#0.render()' to the console (if enabled) and all the elements highlight that are related to the injected property.

As far as I know this wasn't the case before and is not intended?

You can see this by using the https://gist.github.com/ConneXNL/32f65c708efb61af3e3e2d6f2df4aa4c example.

@mweststrate
Copy link
Member Author

@ConneXNL that is a result of making selectors reactive, if you pass a function into inject, it becomes an observer component as well, which is correct as you probably want to see that one reaction as well when needed(?) Might be changed indeed later, but at least not in the 4.0 release directly (there is an open todo / idea for that in the code)

@jrmyio
Copy link

jrmyio commented Nov 17, 2016

I can see that being the case with the console.logging + you are able to disable the logging yourself by applying a filter.

However, I feel that for highlighting the elements it's not a good default to have them blink when the render() method of the component owning the element isn't hit.

@mweststrate
Copy link
Member Author

Shipped with 4.0.0 :)

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

No branches or pull requests

3 participants