frintjs / frint

Modular JavaScript framework for building scalable and reactive applications
https://frint.js.org/
MIT License
755 stars 34 forks source link

Proposal: Access to `app` instance synchronously #424

Open fahad19 opened 6 years ago

fahad19 commented 6 years ago

Currently

Components can only access the app instance via observe HoC, which also involves RxJS for streaming props:

import React from 'react';
import { observe } from 'frint-react';

function MyComponent() {
  return <p></p>;
}

export default observe(function (app, parentProps$) {
  return props$;
})(MyComponent);

Proposal

Not everyone needs to work with streaming props, and may only want to be able to access the app instance (which has the providers), and carry on with regular React lifecycle operations.

To achieve that, we can extend frint-react:

1: withApp function

import React from 'react';
import { withApp } from 'frint-react';

function MyComponent() {
  return <p></p>;
}

export withApp((app, props) => <MyComponent />);

2: WithApp component with render prop

This may not be necessary. Just withApp function alone should be enough.

import React from 'react';
import { WithApp } from 'frint-react';

export default function MyComponent() {
  return (
    <WithApp>
    {
      (app, props) => <p></p>
    }
    </WithApp>
  );
}
fahad19 commented 6 years ago

/cc @leandrooriente, @rbardini, @juliamaksimchik

armand1m commented 6 years ago

I was speaking to @noisae and after some discussion, we thought that an API similar to redux connect would be nice since we could map what we do actually need from the App instead of injecting the whole App always.

This encourages developers using Frint to actually be more specific about what do they actually need from the App instead of relying on the whole app always.

Obviously the developer will still be able to do so, which there is also use cases that we don't have in mind, but implementing this can actually lead us to a path where we can encourage it and know more about what people actually use from their apps.

Perhaps something like:

/** ./MyComponent.js */

import React from 'react';

import FlightSearchStorePropType from '@prop-types/FlightSearchStorePropType';
import CheckoutStorePropType from '@prop-types/CheckoutStorePropType';

import { withApp } from 'frint-react';

const MyComponent = ({
  flightSearchStore,
  checkoutStore,
}) => (
  // render logic
);

MyComponent.propTypes = {
  flightSearchStore: FlightSearchStorePropType.isRequired,
  checkoutStore: CheckoutStorePropType.isRequired,
};

export const mapAppToProps = app => ({
  flightSearchStore: app.get('flightSearchStore'),
  checkoutStore: app.get('checkoutStore'),
});

export default withApp(mapAppToProps)(MyComponent);

This is also good since it makes it easier to test. You can export the mapAppToProps method and validate if it is calling the right resolvers for the expected providers.


/** ./MyComponent.test.js */

import { mapAppToProps } from './MyComponent';

const appMock = {
  get: jest.fn().mockImplementation(name => name),
};

describe('mapAppToProps', () => {
  it('should call correct resolvers', () => {
    mapAppToProps(appMock);
    expect(appMock.get).toBeCalledWith('flightSearchStore', 'checkoutStore');
  });

  it('should map to correct props' () => {
    const props = mapAppToProps(appMock);
    expect(props.flightSearchStore).toBe('flightSearchStore');
    expect(props.checkoutStore).toBe('checkoutStore');
  });
});
fahad19 commented 6 years ago

@armand1m: Your suggestion makes sense, and adopting that kind of convention for Component modules can lead to more maintainable tests.

Actually, the current observe can already do this:

import React from 'react';
import { observe } from 'frint-react';

export const mapAppToProps = app => ({
  foo: app.get('foo'),
});

function MyComponent({ foo }) {
  return <p></p>;
};

export default observe(mapAppToProps)(MyComponent);

One thing that observe doesn't do is giving you access to received props synchronously (it gives you access to parent props as an Observable). That's a problem for people who don't want to get into RxJS. Something we could solve with a new withApp function may be without having to break observe HoC's API.

armand1m commented 6 years ago

@fahad19: Agreed. The API would be pretty straight-forward in that sense,