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

Add ability to provide render callback #858

Closed
wants to merge 3 commits into
base: master
from

Conversation

2 participants
@slorber
Contributor

slorber commented Mar 7, 2018

This render callback is needed for Gatsby integration. (https://github.com/gatsbyjs/gatsby/blob/021a460184be6b8f8e1d2b3d072baabbbe45544c/packages/gatsby/cache-dir/production-app.js#L185)

Not exactly sure what I'm doing so tell me if I did something wrong :)

@necolas

Please could you add a unit test to ensure the callback is called after render, and update the docs for AppRegistry.

@slorber

This comment has been minimized.

Contributor

slorber commented Mar 30, 2018

@slorber

This comment has been minimized.

Contributor

slorber commented Apr 7, 2018

Hi @necolas

I tried to write a test for this callback but I'm not exactly sure how to do this.

All the other tests are currently using Enzyme which does not use any callback, and the following code is currently not tested as far as I've seen:

const renderFn = process.env.NODE_ENV !== 'production' ? render : hydrate;

export default function renderApplication<Props: Object>(
  RootComponent: ComponentType<Props>,
  initialProps: Props,
  rootTag: any,
  callback?: () => void
) {
  invariant(rootTag, 'Expect to have a valid rootTag, instead got ', rootTag);

  renderFn(
    <AppContainer rootTag={rootTag}>
      <RootComponent {...initialProps} />
    </AppContainer>,
    rootTag,
    callback
  );
}

To test the callback addition, this renderApplication function should indeed be tested in the first place. I am ok to write such a test if you want.

Do we agree that the test should not call the real ReactDOM render/hydrate functions and mock them? So basically the test will only verify that the arguments are dispatched correctly to real render/hydrate methods? This means I should then probably pass an extra argument renderFn to renderApplication with a default value to current renderFn so that I can mock it in the test and verify it is called with the callback as 3rd arg. This seems a bit overkill :)

I'll update the doc

slorber added some commits Apr 7, 2018

Merge branch 'master' into add-render-callback
# Conflicts:
#	packages/react-native-web/src/exports/AppRegistry/index.js
#	packages/react-native-web/src/exports/AppRegistry/renderApplication.js
@slorber

This comment has been minimized.

Contributor

slorber commented Apr 7, 2018

Hi @necolas

I've updated the doc and resolved the conflicts of the PR related to the new WrapperComponent

Waiting for your feedback related to the renderApplication test.

Note that I wasn't able to run the storybook website. Do you have a contribution guide explaining that? Should I use Lerna or Yarn should be enough?

@necolas

This comment has been minimized.

Owner

necolas commented Apr 7, 2018

Thanks, I'll take care of adding the test

@necolas necolas closed this in 7a3a9a5 Apr 7, 2018

@necolas

This comment has been minimized.

Owner

necolas commented Apr 7, 2018

Note that I wasn't able to run the storybook website. Do you have a contribution guide explaining that?

https://github.com/necolas/react-native-web/blob/master/.github/CONTRIBUTING.md

@slorber

This comment has been minimized.

Contributor

slorber commented Apr 7, 2018

Thanks, I'll upgrade the Gatsby plugin as soon as there's a release (ping me)

I tried yarn website but got errors, it was because I didn't know I had to compile first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment