Skip to content
This repository has been archived by the owner on Aug 19, 2024. It is now read-only.

fix(optimization): limit fields retrieval for user #188

Merged
merged 1 commit into from
Jun 1, 2018

Conversation

jamiter
Copy link
Contributor

@jamiter jamiter commented Jun 1, 2018

This was suggested at apollographql/meteor-integration#118

This package, purposely, only adds a userId, so the developer can decide themselves what else they want to fetch from the user. This means we only need to retrieve the ID and the resume tokens for validation.

If the developer wants to add user data to the context, they can simply add it via a custom context:

function context (currentContext) {
  const newContext = { ...currentContext };

  if (currentContext.userId) {
    newContext.user = Meteor.users.findOne(currentContext.userId, { fields: { /* whatever */ } });
  }

  return newContext;
}

One might argue that this would cause 2 requests instead of one, but in my opinion it's way more flexible and will only actually retrieve the data that is required. 2 small requests might still be faster then one large? Besides, you might only need user data in certain resolvers, so it might be even better to only query in those resolvers (using DataLoader for deduping). userId says something about authentication. User data should be retrieved like you would with any other data.

In this particular package it's also the case that DDP only offers a userId. By only offering a userId in the HTTP flow we can handle both the same way.

This all also prevents issues like mentioned here: apollographql/meteor-integration#119

@jamiter jamiter self-assigned this Jun 1, 2018
Copy link

@quintstoffers quintstoffers left a comment

Choose a reason for hiding this comment

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

We can always make the fields configurable in the future and pass on the fetched user in the future.

@jamiter
Copy link
Contributor Author

jamiter commented Jun 1, 2018

@quintstoffers, that wouldn't solve apollographql/meteor-integration#119 and would make authorizing the request less standard (just getting the userId) and make the DDP and HTTP version different. So I think we're good for now 😉

@jamiter jamiter merged commit 6ff01f3 into master Jun 1, 2018
@jamiter jamiter deleted the fix/limit-fields-retrieval-for-user branch June 1, 2018 09:40
jamiter pushed a commit that referenced this pull request Jun 1, 2018
This makes sure we only get the userId and no other details from the
user. That is something for developers to decide and more in line with
DDP itself. See reasoning in
#188
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.

2 participants