Skip to content
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

Expose injectCacheManager on RelayEnvironment #1320

Closed
wants to merge 2 commits into from

Conversation

aweary
Copy link
Contributor

@aweary aweary commented Aug 3, 2016

RelayEnvironment exposes most of the other injection methods for RelayStoreData, so this just adds injectCacheManager as well. Even though it's not a documented API, adding it to RelayEnvironment will make it easier for those who do want to use it while its "experimental" which paves the way for a stable implementation.

I also added a basic test that just verifies the injection works as expected.

@ghost ghost added the CLA Signed label Aug 3, 2016
it('passes down the cacheManager to the store data', () => {
const mockCacheManager = {};
environment.injectCacheManager(mockCacheManager);
expect(environment._storeData._cacheManager).toEqual(mockCacheManager);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expect(environment.getStoreData().getCacheManager()).toBe(mockCacheManager);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, forgot about getStoreData() and getCacheManager(). Fixed 👍

@aweary aweary force-pushed the expose-inject-cache-manager branch from 06c1eb5 to 4ce48ea Compare August 3, 2016 23:11
@ghost ghost added the CLA Signed label Aug 3, 2016
@josephsavona
Copy link
Contributor

@facebook-github-bot shipit

@ghost
Copy link

ghost commented Aug 4, 2016

Thanks for importing.If you are an FB employee go to Phabricator to review internal test results.

@ghost ghost added the CLA Signed label Aug 4, 2016
@aweary
Copy link
Contributor Author

aweary commented Aug 4, 2016

@josephsavona do you know why the test is failing? is RelayStoreData being mocked or something?

@ghost ghost added the CLA Signed label Aug 4, 2016
@josephsavona
Copy link
Contributor

Not sure, I would try logging some of the values and see.

const mockCacheManager = {};
environment.injectCacheManager(mockCacheManager);
expect(
environment.getStoreData().getCacheManager()
Copy link
Contributor

@edvinerikson edvinerikson Aug 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getCacheManager don't seem to exist (there is no such method in the RelayStoreData class)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird, thought we had that. Anyway, let's add and use it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add it in this PR if you'd like

@josephsavona
Copy link
Contributor

@facebook-github-bot shipit

@ghost
Copy link

ghost commented Aug 8, 2016

Thanks for importing.If you are an FB employee go to Phabricator to review internal test results.

@ghost ghost added the CLA Signed label Aug 8, 2016
@ghost ghost closed this in e9e3b24 Aug 8, 2016
@ghost ghost added the CLA Signed label Aug 8, 2016
@josephsavona
Copy link
Contributor

@aweary thanks for this!

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants