Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Check if we can remove the export of 'analytics' object #3

Open
itaym2 opened this issue Sep 6, 2017 · 2 comments
Open

Check if we can remove the export of 'analytics' object #3

itaym2 opened this issue Sep 6, 2017 · 2 comments

Comments

@itaym2
Copy link
Contributor

itaym2 commented Sep 6, 2017

currently the analytics object is exposed because it was being used in our app. We need to see if we can stop exposing it, or maybe it needs to be created in the app and injected to the library.

@AvivRubys
Copy link
Contributor

AvivRubys commented Jan 17, 2018

If we use a Provider style, like redux, we should be able to avoid this.
Analytics has three abilities:
dispatcher - Get current root dispatcher. Not sure anyone even uses this besides internal usage, but shouldn't be needed anyway.
setWriter - Set the event writer, this is set inside the root dispatcher, so shouldn't be needed.
transformDispatcher - This is the hard part. We need to transform and modify the existing root dispatcher. This shouldn't be supported in react-shisell, but the app itself should update the root dispatcher and update the dispatcher given to the Provider.
So what I'm envisioning is this:

import {createRootDispatcher} from 'shisell';
import {AnalyticsProvider} from 'react-shisell';

const writer = event => console.log('Analytic sent', event);
const dispatcher = createRootDispatcher(writer);

const Root = () => (
  <AnalyticsProvider dispatcher={dispatcher}>
    <MyApp />
  </AnalyticsProvider>
);

ReactDOM.render(<Root />, ...);

This shouldn't be a big change, we can include it as a part of 1.0.0

@AvivRubys
Copy link
Contributor

So I've given this some more thought, it's not as simple as I thought it was.
The problem is transformDispatcher - when using a Provider, it needs to both be accessible as a module, and be able to access a react component's state.
I see three main options.

  1. Leave it as it is, but move the global dispatcher creation into the app, and inject it into the library using a setRootDispatcher.
  2. Create a singleton component whose module will export a component and a transformDispatcher function. That way you can still import transformDispatcher and the component independently.
  3. Create a component that passes down in context both a dispatcher and the transformDispatcher function. This will be fairly clean, but will mean that we could only access it from a react context.

I'm gonna go with option number 2 for now and see if I run into anything major, because it seems like the best balance of hackiness to good APIs.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants