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

Reduce amount of queries for Teams feature #708

Merged
merged 1 commit into from
Mar 17, 2021

Conversation

lexdewilligen
Copy link
Contributor

The inertia middleware which is used to share data to the frontend currently runs 3 queries to load all teams and the current team.

  1. Load all owned teams
  2. Merge all other teams
  3. Load the currentTeam

However, this PR removes the last query since the currentTeam is one of the teams that's loaded in allTeams. This might just be a slight performance improvement! I hope it's worth merging into Jetstream.

@taylorotwell taylorotwell merged commit 8028279 into laravel:2.x Mar 17, 2021
Comment on lines +45 to +47
$request->user()->setRelation('currentTeam', $request->user()->allTeams()->filter(function ($team) use ($request) {
return $team->id === $request->user()->current_team_id;
})->first());
Copy link
Contributor

@matthieumota matthieumota Mar 24, 2021

Choose a reason for hiding this comment

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

Hi,

I think that PR cause a bug due to current_team_id field who can be null. Just try to install a fresh Laravel with Jetstream and you're unable to login (white page due to JS error and team).

It's because field is refresh with switchTeam method. Before PR, currentTeam() was called and check that.

Maybe you can try that 👍

if (is_null($request->user()->current_team_id)) {
    $request->user()->switchTeam($request->user()->personalTeam());
}

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, reverted.

@driesvints
Copy link
Member

@lexdewilligen this broke login/registration for Inertia, see #708 (comment)

I've reverted this and tagged a new release. Next time, please try to test you change thoroughly before sending this in.

@lexdewilligen
Copy link
Contributor Author

@driesvints I did test it thoroughly, but I must have missed it! I'm very sorry this has happened!

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.

4 participants