Skip to content

Conversation

@benmccann
Copy link
Member

@benmccann benmccann commented Sep 20, 2022

It's not pretty, but it works

@benmccann benmccann force-pushed the upgrade branch 2 times, most recently from badf42d to db91603 Compare September 26, 2022 21:33
set(value) {
return getStores().user.set(value);
}
};
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't feel like the sort of code we want to be encouraging either. It's very confusing.

We're already exposing the user via data from +layout.server.js. We should just access the user everywhere via $page.data.user, not with this special store.

Copy link
Member Author

@benmccann benmccann Sep 27, 2022

Choose a reason for hiding this comment

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

The first commit in this PR used data.user everywhere, but it was a bit problematic because the only way to update it was to re-request the data. So if you want to do something like update the user display name you have to make one request to set it and then make a second request to get the data eventhough we already have it.

We had talked at one point about exposing SvelteKit's version of this so that users don't have to write special store handling code themselves. I think that would be a nice thing to do. Even if we don't use a store here, users do need to jump through some hoops if they ever want to use a store in their application for any other purpose

Copy link
Member Author

@benmccann benmccann Sep 27, 2022

Choose a reason for hiding this comment

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

Another solution would be to make $page.data writable. If it were, then I could simply update it instead of needing to create a new store. That would also solve the remaining problem, which is that load is receiving an outdated version of data.user after login.

@pepegc
Copy link
Contributor

pepegc commented Oct 12, 2022

@benmccann I read in #137 that you are waiting for new features to be introduced before merging this. Do you have a timeline for these features? I know Removing session #5883 generated some backlash.

</li>

{#if $session.user}
{#if $user}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think under this new solution this evaluates as true all the time.

Suggested change
{#if $user}
{#if $user.username}

@Rich-Harris Rich-Harris mentioned this pull request Nov 3, 2022
@Rich-Harris
Copy link
Member

Started work on a more comprehensive rewrite here: #139. I don't really understand what all the weird stores code is for here tbh!

@benmccann
Copy link
Member Author

I'll go ahead and close this

@benmccann benmccann closed this Nov 4, 2022
@benmccann benmccann deleted the upgrade branch November 4, 2022 02:20
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.

5 participants