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

Local fetch option in WatchQueryOptions #385

Merged
merged 13 commits into from
Jul 14, 2016
Merged

Local fetch option in WatchQueryOptions #385

merged 13 commits into from
Jul 14, 2016

Conversation

amandajliu
Copy link
Contributor

@amandajliu amandajliu commented Jul 11, 2016

Fixes #225

TODO:

  • Update CHANGELOG.md with your change
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

@@ -88,7 +96,7 @@ export function readSelectionSetFromStore({
selectionSet,
rootId,
store,
throwOnMissingField: !returnPartialData,
throwOnMissingField: !returnPartialData && !noFetch,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to pass through noFetch everywhere just to do this? I feel like we could just set returnPartialData: true in QueryManager or something instead of adding a new option.

@stubailo
Copy link
Contributor

IMO we should never have a function or option that silently doesn't do the thing you want. It should either do something reasonable or throw an error. Since it's not clear how this option should interact with polling or refetch, I'd suggest those should just not be usable together with noFetch.

…eQuery so its functions are defined within itself given query options
@@ -97,7 +97,7 @@ export class QueryScheduler {
throw new Error('Tried to register a non-polling query with the scheduler.');
}

const queryId = this.queryManager.generateQueryId();
/*

const subscriberFunction = (observer) => {
Copy link
Contributor Author

@amandajliu amandajliu Jul 12, 2016

Choose a reason for hiding this comment

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

@Poincare when replacing QueryManager's "watchQuery" with scheduler's equivalent pass this subscriberFunction into ObservableQuery

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok cool. I think the scheduler will just register the polling query and use QueryManager to construct the ObservableQuery.

@@ -97,7 +97,7 @@ export class QueryScheduler {
throw new Error('Tried to register a non-polling query with the scheduler.');
}

const queryId = this.queryManager.generateQueryId();
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Somehow I don't think this should be commented out? Should we delete the code?

@stubailo
Copy link
Contributor

This looks pretty good to me overall, just some issues with commented code and not sure the test is correct. Also, we should rebase this on master just in case.

@@ -8,6 +8,8 @@ Expect active development and potentially significant breaking changes in the `0
- Stringify `storeObj` for error message in `diffFieldAgainstStore`.
- Fix map function returning `undefined` in `removeRefsFromStoreObj`. [PR #393](https://github.com/apollostack/apollo-client/pull/393)

- Added a "noFetch" option to WatchQueryOptions that only returns available data from the local store (even it is incomplete). [Issue #225] https://github.com/apollostack/apollo-client/issues/225 and [PR #385]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a valid markdown link :]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops

@stubailo stubailo deleted the local-fetch branch September 20, 2016 03:43
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants