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

Realworld update #290

Merged
merged 30 commits into from
Mar 25, 2021
Merged

Realworld update #290

merged 30 commits into from
Mar 25, 2021

Conversation

Rich-Harris
Copy link
Member

No description provided.

Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

looks good to me. As a heads up, the build is failing. Looks like you might need to update pnpm-lock.yaml

packages/kit/src/renderer/page.js Outdated Show resolved Hide resolved
packages/kit/src/renderer/page.js Outdated Show resolved Hide resolved
@@ -0,0 +1,28 @@
const noop = () => {};

export function ajax(node, { onsubmit = noop, onresponse = noop } = {}) {
Copy link
Member

Choose a reason for hiding this comment

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

I somewhat wonder if this would be a nice helper to have in SvelteKit. (I might call it formHandler instead of ajax)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think something like this should be easy to reuse, though I don't know if this is exactly the right thing — it does some slightly opinionated stuff (like setting the accept header, so that a no-JS form submission can get a 303 response while an AJAX submission gets the posted data), and we'd need to be more careful about the design if it wasn't just a little helper inside one of the example apps.

But yeah, would be nice to have some version of this in the toolkit, though maybe in Svelte itself rather than SvelteKit-specific (sveltejs/rfcs#24)

@Rich-Harris
Copy link
Member Author

i guess there's one major downside of storing the whole JWT in the cookie — if you change user details in one browser (e.g. desktop) they won't be reflected when you log in via another (e.g. mobile). So I guess we have to just store the token and communicate with the realworld API to get user data for each request

@Rich-Harris
Copy link
Member Author

this is currently broken, am working on getting it finished (or at least 'acceptable')

@Rich-Harris Rich-Harris changed the title [WIP] Realworld update Realworld update Mar 25, 2021
@Rich-Harris
Copy link
Member Author

No doubt there's more that can be done here but I think it makes sense to merge this in its current state

@Rich-Harris Rich-Harris merged commit 52f5a22 into master Mar 25, 2021
@Rich-Harris Rich-Harris deleted the realworld-update branch March 25, 2021 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants