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

[0.5.x] Add svelte 5 #104

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

[0.5.x] Add svelte 5 #104

wants to merge 10 commits into from

Conversation

renatomoor
Copy link

Add support for svelte 5

@renatomoor renatomoor changed the title Add svelte 5 package Add svelte 5 Oct 28, 2024
@taylorotwell
Copy link
Member

Thanks for your work on this! We may need someone with more Svelte experience to review 😅

cc @pedroborges - do you have anytime to give this a look?

@taylorotwell taylorotwell marked this pull request as draft October 29, 2024 14:07
@joshcirre
Copy link

Hey @renatomoor, just giving some thoughts here. Let me know if they make sense or if I'm misunderstanding anything.

I think the goal would likely trying to get it closer in resemblance to the React or Vue versions. The biggest difference I can see is how you aren't using the resolveName in the validate method. Is there a reason for that?

Additionally, I think the submit method should be async to match other versions.

This one might be nitpicky, but I don't think the getters for the end of the form function are needed, no? The get validating() function could just be passed in as a parameter? Then in use, it would be $form.submit() or {$form.errors.name} for example.

Lastly, I think the hasErrors being the new $derived rune from Svelte 5 makes sense since it would be derived from the errors value?

Something like:

const hasErrors = $derived(Object.keys(errors).length > 0)

Let me know if that all makes sense!

@renatomoor
Copy link
Author

renatomoor commented Oct 30, 2024

Hello @joshcirre, Thanks for the feedback!

Great catch on resolveName and submit. I've fixed those.

Regarding the getters, they’re part of the new approach in Svelte 5 for getting reactive values from outside Svelte components, as the Svelte team explained here: https://svelte.dev/blog/runes#beyond-components.

I initially started using $form while working on the PR with Svelte 4. However, with Svelte 5, there’s no need to prefix reactive variables with $. My idea was to make it similar to other frameworks like Vue and React:

<script>
    let form = useForm('post', 'register', {
        name: '',
        email: '',
        password: '',
        password_confirmation: '',
    })
</script>


<span>Validating: {form.validating}</span> <!-- Here we are getting the reactive value defined outside the component with $state through the getter -->
<span>Processing: {form.processing}</span>
<span>hasErrors: {form.hasErrors}</span>

<Form.Input
    bind:value={form.data.name}
    error="{form.errors.name}"
    label="Name"
    name="name"
    on:change={() => form.validate('name')}
    placeholder="Name"
    required
    type="text"
/>

<!-- ... -->

<Button on:click={() => form.submit()} type="submit">Register</Button>

As for hasErrors, the other packages for Vue and React don’t use computed properties but instead rely on a simple reactive variable. So, I kept it consistent with those frameworks.

That said, this is actually my first time working with Svelte 4 or 5, so please let me know if there’s anything else to check!

I’ve been testing it in SvelteKit, and it’s working fine, though I’m sure there could be further improvements. I considered using {form.name} instead of {form.data.name}, but I encountered multiple errors when trying to implement it, so I stuck with the React approach by using .data.

@joshcirre
Copy link

Hey @renatomoor, makes perfect sense! I like the changes, and I agree that while it might make sense from a Svelte-ish syntax to use $derived, I do agree that keeping it as similar to the React/Vue implementations is best, and I think these changes fall in line with that.

I just tried it out in a SvelteKit project and it works great!

(I also like the .data approach just for consistency sake)

@pedroborges
Copy link

Hey @taylorotwell, I can test this out and provide a full review over the weekend. Started at a new job this week and have been pretty busy 😃

@renatomoor and @joshcirre, I have a proof of concept for the Inertia.js Svelte adapter using Svelte 5 that I'm not ready to share just yet. This PR aligns with my direction by utilizing one $state per property 💯 But instead of making the data property publicly accessible, I’ve added getters and setters for each data property on the form object, allowing usage like:

let form = useForm('post', {
    name: '',
    email: '',
    password: '',
    password_confirmation: '',
})

console.log(form.name)
console.log(form.email)
console.log(form.password)
console.log(form.password_confirmation)

This approach keeps consistency with the current Svelte adapter. If want to give it a shot:

const form = {
  data() {
    return $state.snapshot(data) as TForm
  },

  // ...
}

// Define "data" getters and setters on "form"
(Object.keys(data) as Array<keyof TForm>).forEach((key) => {
  Object.defineProperty(form, key, {
    get() {
      return data[key]
    },
    set(value: TForm[keyof TForm]) {
      data[key] = value
    },
  })
})

return form

I agree with @joshcirre on using $derived for hasErrors, that's exactly what I'm doing. I’ll test this out and share any further feedback soon. Looking forward to everyone’s thoughts 😉

@pedroborges
Copy link

pedroborges commented Oct 31, 2024

I just noticed Inertia.js isn’t included in this PR. @renatomoor are you familiar with Inertia.js and able to add the adapter as well? If not, no worries. I can take care of it after your PR is merged.

Thanks again for your awesome work!

Copy link

@pedroborges pedroborges left a comment

Choose a reason for hiding this comment

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

Great work so far, @renatomoor! 🎉

Please take a look at my comments and questions below. I’m here to help or answer any questions you might have.

I haven’t had a chance to run the code yet since I don’t have any project using Precognition. If you have a demo project you can share, I’d love to test it out!

packages/svelte/src/types.ts Outdated Show resolved Hide resolved
packages/svelte/src/types.ts Outdated Show resolved Hide resolved
packages/svelte/src/index.svelte.ts Outdated Show resolved Hide resolved
packages/svelte/src/index.svelte.ts Show resolved Hide resolved
packages/svelte/package.json Show resolved Hide resolved
packages/svelte/src/index.svelte.ts Outdated Show resolved Hide resolved
@renatomoor
Copy link
Author

@pedroborges thanks for the comments!
I'll take a look at all today.
I did not had time during the weekend but this week I should have some hours free to work on this.

@renatomoor
Copy link
Author

hey! @pedroborges @joshcirre.

I just did the repo to test laravel precognition with svelte kit: https://github.com/renatomoor/sveltekit-precognition

Later today I'll implement the changes that you both requested.

@renatomoor
Copy link
Author

@pedroborges I’ve made the corrections for the Svelte package. You can check them again!

I’ll continue with the Inertia package tomorrow or the day after.
I’d prefer to create a separate PR for that one.

WDYT?

@timacdonald
Copy link
Member

I'd like to see the Inertia version here before we look at merging this.

The API decisions made here should be based the Inertia Svelte adapter. If we implement this version first and then find out there are inconsistencies in the Inertia version, that is gonna be annoying to maintain.

@timacdonald timacdonald changed the title Add svelte 5 [0.5.x] Add svelte 5 Nov 7, 2024
@timacdonald
Copy link
Member

Would anyone be kind enough and have time to spin up a bare bones svelte project with a basic form for me to test this out in? Even if it is just the front-end component. I can handle the Laravel side of things if that helps.

@renatomoor
Copy link
Author

Hello @timacdonald!

I did that already here #104 (comment).

I'll see if I can do tomorrow or the day after the inertia part. 😉

@timacdonald
Copy link
Member

You rock ❤️

@renatomoor
Copy link
Author

@timacdonald or @pedroborges, Is there a repo with inertia and svelte 5 ?

I've worked with inertia and vue + precognition in the past but never with svelte, so if you can guide me a little bit here would be nice!

@pedroborges
Copy link

@renatomoor
Copy link
Author

Thanks @pedroborges, I saw your package a few days ago, but I have not had the time yet to develop the inertia part this week.

@joaopalopes24
Copy link
Contributor

Add svelte-inertia too, please.

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.

6 participants