Skip to content

Add include option to Login #4845

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

Closed
wants to merge 4 commits into from

Conversation

dplewis
Copy link
Member

@dplewis dplewis commented Jun 20, 2018

#288

Parse previously didn't have any way to get all pointer data a User class might have on login, unnecessarily requiring an additional call to get full pointer objects.

Let me know if there is a better way to implement it.

@flovilmart
Copy link
Contributor

Sorry but I really hate it :(

@dplewis
Copy link
Member Author

dplewis commented Jun 20, 2018

I noticed there were a lot of things going on the the User class as well. There may be a lot of issue that would arise in the future.

Which part do you dislike?

@flovilmart
Copy link
Contributor

That it’s kinda messy... the login method is to acquire a sessionToken, the fact there’s a user attached is actually a ‘side effect’. The thing that matters on login is not the user but the token....

@dplewis
Copy link
Member Author

dplewis commented Jun 20, 2018

Ah I see your point, makes sense.

@flovilmart
Copy link
Contributor

While I agree this is a bit shitty, that post login, you may wanna run a fetch with includes.

Which makes me think, that we don’t support fetch with includes in the SDK. What do you say we add it?

@dplewis
Copy link
Member Author

dplewis commented Jun 20, 2018

If you use include you don't have to fetch right?

I'm currently using fetch without include on user login on iOS. Thats why I made this PR.

Ive had it this way for a few years and early on I ran into deadlock issues when I went to refresh users. It always bugged me that I had to use fetch.

@flovilmart
Copy link
Contributor

What I mean, is that on iOS, calling object.fetch() doesn’t let you specify a list of keys to include.

@dplewis
Copy link
Member Author

dplewis commented Jun 20, 2018

Oh, thats interesting. How do you see it being implemented in the SDK, the fetch with keys?

Also I always wondered is fetch faster than query.get()

@flovilmart
Copy link
Contributor

Those are the exact same calls BUT, the query let you be a bit more expressive with includes

@codecov
Copy link

codecov bot commented Jun 20, 2018

Codecov Report

Merging #4845 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4845      +/-   ##
==========================================
+ Coverage   92.81%   92.82%   +0.01%     
==========================================
  Files         119      119              
  Lines        8813     8827      +14     
==========================================
+ Hits         8180     8194      +14     
  Misses        633      633
Impacted Files Coverage Δ
src/Routers/UsersRouter.js 94.19% <100%> (+0.57%) ⬆️

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 5cdbde2...0492df4. Read the comment docs.

@dplewis dplewis mentioned this pull request Jun 21, 2018
8 tasks
@dplewis dplewis requested a review from acinader June 25, 2018 22:24
@dplewis
Copy link
Member Author

dplewis commented Jul 12, 2018

@flovilmart Should we close this one?

@flovilmart
Copy link
Contributor

Let’s not close it :) in the end, it may be useful if we add it with the login options .

@stale
Copy link

stale bot commented Oct 19, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

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

Successfully merging this pull request may close these issues.

3 participants