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 <NavigationEvents/> component #4188

Merged
merged 14 commits into from Jun 29, 2018

Conversation

Projects
None yet
7 participants
@slorber
Contributor

slorber commented May 10, 2018

Declarative component to register event listeners, as discussed in RFC react-navigation/rfcs#41

@react-navigation-ci

This comment has been minimized.

@react-navigation-ci

This comment has been minimized.

@slorber

This comment has been minimized.

Contributor

slorber commented May 10, 2018

@brentvatne @satya164 @ericvicenti here is the PR as discussed in RFC.

  1. There is no test. Any advice on how to test that? I'll try to mock the navigation/addListener and see existing tests related to event system

  2. There is no doc yet

  3. I noticed a weird behavior on my example. I'm not sure it's related to my code but the 2nd time we enter the NavigationEvents example, the events are not fired anymore. I suspect it's something broken in the lib because the "emit()" function is not even called so it does not look like an event listener registration problem

@slorber

This comment has been minimized.

Contributor

slorber commented May 10, 2018

Hmmmm it seems the 3) issue with events not firing has been fixed in a recent commit, after updating from upstream the problem just disappeard :)

@react-navigation-ci

This comment has been minimized.

@slorber

This comment has been minimized.

Contributor

slorber commented May 10, 2018

I've added some tests + flow declaration.

If someone can check the flow declaration that could be nice, not exactly sure what I'm doing.

I'll do the documentation in another PR as it's out of this repository.

Should I support the "action" listener? I've seen this in the code but it's not documented.

Should I support case where the provided navigation prop change over time (ie unregister all listeners from navigation1, and re-register them on navigation2?)

Not sure exactly why but I have an unrelated test that can't pass:

image

@react-navigation-ci

This comment has been minimized.

@slorber slorber changed the title from [WIP] Add <NavigationEvents/> component to Add <NavigationEvents/> component May 10, 2018

EventNames.forEach(this.removeListener);
}
addListener = eventName => {

This comment has been minimized.

@vonovak

vonovak May 11, 2018

Collaborator

as an alternative, we could consider

addListener = eventName => {
  this[eventName] = (...args) => {
    const listener = this.props[EventNameToPropName[eventName]];
    listener && listener(...args)
  }

  this.subscriptions[eventName] = this.props.navigation.addListener(
    eventName,
    this[eventName]
  );
};

Or, more verbosely (to show what I mean):

class NavigationEvents extends React.Component {
  willBlur = (...args) => {
    const { onWillBlur } = this.props;
    onWillBlur && onWillBlur(...args);
  };

  addListenerForWillBlur = () => {
    this.subscriptions.willBlur = this.props.navigation.addListener('willBlur', this.willBlur);
  };
}

This would allow to remove componentDidUpdate and its remove + add listener calls. Just subscribing once in componentDidMount, unsubscribing in componentWillUnmount and not caring about the changes of the props of NavigationEvents.

This comment has been minimized.

@slorber

slorber May 14, 2018

Contributor

@vonovak I've proposed a similar implementation initially, using "internal listeners", but @satya164 suggested I subscribe user-provided listeners directly here

I'm ok for your proposal, the only drawback is that if user provides a single callback, you would have 4 listeners being subscribed, with 3 of them doing nothing. Not a big deal IMHO so if we settle on that I'll make the change.

This comment has been minimized.

@ericvicenti

ericvicenti May 24, 2018

Contributor

Hmm, I think @satya164's suggestion makes a lot of sense and this implementation is quite good. It may be a tiny bit harder to read but it prevents creation of unnecessary upstream subscriptions

@react-navigation-ci

This comment has been minimized.

@slorber

This comment has been minimized.

Contributor

slorber commented May 24, 2018

Hi @brentvatne @ericvicenti

Any feedback on the PR?
What about the implementation comment of @vonovak ?
Should I start writing documentation about the component?

Just rebased it on upstream, all tests passing on CI except the one that is broken on master:

NavigationContainer › warnings › detached navigators › warns when you render more than one navigator explicitly

Also this is weird but locally I get this test failing (not on CI), any idea?

image

@ericvicenti

Mostly looking great! I'd say this is good enough to merge and then keep iterating on. No real urgency to re-do the test or example right now, but it is something to keep in mind for your next PR or if you want to follow up with improvement on this code.

Thanks @slorber for the contribution!

const EventNames = Object.keys(EventNameToPropName);
class NavigationEvents extends React.Component {
constructor() {

This comment has been minimized.

@ericvicenti

ericvicenti May 24, 2018

Contributor

delete this?

EventNames.forEach(this.removeListener);
}
addListener = eventName => {

This comment has been minimized.

@ericvicenti

ericvicenti May 24, 2018

Contributor

Hmm, I think @satya164's suggestion makes a lot of sense and this implementation is quite good. It may be a tiny bit harder to read but it prevents creation of unnecessary upstream subscriptions

});
it('attach all listeners with navigation context', () => {
const component = renderer.create(<TestComponent />);

This comment has been minimized.

@ericvicenti

ericvicenti May 24, 2018

Contributor

these tests seem to do the job, but personally I'm a fan of avoiding much work in beforeEach and instead making it so that each test reads more like a standalone story. The way you have it here, I need to read the whole complicated test harness to understand a single test. The "story" strategy results in longer test files, but I think the improved readability makes it worthwhile.

I'd say its fine for now but in the future you may want to keep the whole test isolated to within the it( callback

/>
</View>
<NavigationEvents

This comment has been minimized.

@ericvicenti

ericvicenti May 24, 2018

Contributor

this example looks great for core developers like ourselves who want to test the library, but its a terrible example of how events might be used in practice. This is fine for now, but in the future I would recommend making an example for each purpose.

This comment has been minimized.

@slorber

slorber May 25, 2018

Contributor

Yes I agree. Maybe I could make an example for the scenario where you want to refresh data on focus, but not if this is a focus triggered by a back navigation? (like it was my original usecase)

Do you think of other meaningful/general patterns that make sense with event?

This comment has been minimized.

@satya164

satya164 May 25, 2018

Collaborator

refresh data on focus, but not if this is a focus triggered by a back navigation

So basically on mount?

This comment has been minimized.

@slorber

slorber May 25, 2018

Contributor

Simplified usecase:

  • I have a tab navigation with 3 tabs containing small lists (scrollable) of items each.
    All items can be pressed and open a new item detail stacknav screen on top of the scrollable tab screen (like a popup)
  • I want tabs data to refresh on tab focus (mount, but also swipes/tab press), but don't want to refresh in case of a back event from the items detail screen, because this would mess-up with the scroll position.
@ericvicenti

This comment has been minimized.

Contributor

ericvicenti commented May 24, 2018

As for the local test failure, I haven't seen that yet.. It looks like the sort of issue that might get fixed by wiping out your node_modules and re-yarn

@react-navigation-ci

This comment has been minimized.

@slorber

This comment has been minimized.

Contributor

slorber commented May 25, 2018

Hi @ericvicenti thanks for the review. I made changes to the PR:

  • removed the useless constructor
  • refactored the test to avoid beforeEach. There are still some helper functions to avoid some duplication, but this should be more easy to read

The local failure persists after rm -rf node_modules + yarn install

@react-navigation-ci

This comment has been minimized.

@ericvicenti

This comment has been minimized.

Contributor

ericvicenti commented May 25, 2018

Ah, it looks like these flow errors are legitimate. I can reproduce them locally with the following:

cd examples/NavigationPlayground
yarn
yarn test
@react-navigation-ci

This comment has been minimized.

@codecov-io

This comment has been minimized.

codecov-io commented May 25, 2018

Codecov Report

Merging #4188 into master will increase coverage by 0.23%.
The diff coverage is 89.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4188      +/-   ##
==========================================
+ Coverage   67.59%   67.83%   +0.23%     
==========================================
  Files          56       57       +1     
  Lines        1725     1744      +19     
==========================================
+ Hits         1166     1183      +17     
- Misses        559      561       +2
Impacted Files Coverage Δ
src/react-navigation.js 14% <0%> (-0.29%) ⬇️
src/react-navigation.web.js 0% <0%> (ø) ⬆️
src/views/NavigationEvents.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e384f8...b614bad. Read the comment docs.

@slorber

This comment has been minimized.

Contributor

slorber commented May 25, 2018

Thanks @ericvicenti just fixed the problem, didn't know there was another command to run playground tests. Assumed yarn test at the root does test everything

@brentvatne

This comment has been minimized.

Member

brentvatne commented May 25, 2018

@ericvicenti - let's discuss whether this is a good fit for the library itself or if it should live in a 3rd party lib

@slorber

This comment has been minimized.

Contributor

slorber commented May 26, 2018

Hi @brentvatne

I feel like this component is of the same kind as withNavigationFocus HOC: it can be built in userland yet is convenient enough to be useful for many. If it were to be put in a react-navigation-extras package or something, maybe it's worth keeping these 2 components togethers so that NavigationEvents does not feel too lonely ;)

@slorber

This comment has been minimized.

Contributor

slorber commented Jun 28, 2018

@ericvicenti @brentvatne have you decided where this component should live yet?

@brentvatne

This comment has been minimized.

Member

brentvatne commented Jun 28, 2018

yeah - @ericvicenti believes this would be a good fit for react-navigation itself, so let's ship it. are there any outstanding changes you would like to make first?

@react-navigation-ci

This comment has been minimized.

@slorber

This comment has been minimized.

Contributor

slorber commented Jun 28, 2018

great :)

nothing to add for me, except planning a new PR for some doc

also, it seems the build is now failing, not sure it's related to the code itself

@react-navigation-ci

This comment has been minimized.

@brentvatne brentvatne merged commit 1951a3a into react-navigation:master Jun 29, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@slorber

This comment has been minimized.

Contributor

slorber commented Jun 29, 2018

great!

about the doc, should I just create a new api reference page in .md with some frontmatter and that's all needed?

I mean copy and modify a page like this one: https://github.com/react-navigation/react-navigation.github.io/blob/45c3e0279e50e07e8701442b15f4df2f26e19828/docs/stack-actions.md

@brentvatne

This comment has been minimized.

Member

brentvatne commented Jun 29, 2018

ya and add to sidebars.yml (I think, afk)

@slorber

This comment has been minimized.

Contributor

slorber commented Jul 9, 2018

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