-
Notifications
You must be signed in to change notification settings - Fork 471
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
[Svelte] Improve typescript types + minor fixes #1881
Conversation
@@ -2,17 +2,15 @@ | |||
import Render, { h } from './Render.svelte' | |||
import store from '../store' | |||
|
|||
$$props |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does nothing
$: { | ||
if (prevComponent !== component) { | ||
key = Date.now() | ||
prevComponent = component | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prior to the typescript merge we had
$: {
if (prevComponent !== component) {
key = Date.now()
prevComponent = component
}
}
and with the typescript merge we are now having
function updateKey(component: InertiaComponentType) {
if (prev !== component) {
prev = component
key = new Date().getTime()
}
}
$: updateKey(component)
While it looks like to be the same, it ain't. Everything between ${...} is reactive and is able to trigger state update unlike when it's wrapped into a method. Maybe it doesn't matter, however I don't see any justification for that change
import type { Page } from '@inertiajs/core' | ||
export type SSRProps = { id: string; initialPage: Page } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No changes here. Exporting the types to use in createInertiaApp for the SSR instead of casting to any for better typing.
@@ -184,8 +174,8 @@ export default function useForm<TForm extends Record<string, unknown>>( | |||
return options.onStart(visit) | |||
} | |||
}, | |||
onProgress: (event: AxiosProgressEvent) => { | |||
this.setStore('progress', event) | |||
onProgress: (event?: AxiosProgressEvent) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the real inertia@core signature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jamesst20, thanks for this PR! @reinink asked me to give it a review, I have just a couple of items that need clarification whenever you have a moment.
What would be the next step? |
Hey guys @joetannenbaum @reinink it's me again bothering you 😆 let me know if I can be of any help |
Hey @jamesst20! Sorry for the delay here. This is on my list to review again tomorrow, I'll update you then. |
This is looking good! Thanks @jamesst20. Just one final nit, let me know what you think. |
@joetannenbaum @reinink @jamesst20 Have anything to do more about this? |
@joetannenbaum I applied the styling suggestion and rebased on the latest commit too
Not really, I have been running my own typescript fork for few months without troubles |
@reinink Could you please merge? :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great work @jamesst20, thank you! Please see my comments.
After installing @types/lodash
I got different type errors. I worked on a fix to those error and have opened a PR to your branch. Please review it when you get a chance.
@pedroborges All done! Nice addition to remove all the duplicated calls and checks for the data type being function or object :) Ready to merge? |
@jamesst20 please install After that, I'd like to see if @joetannenbaum wants to review this once again. Then it will be good to merge. Thanks again @jamesst20! |
Done! @joetannenbaum @pedroborges @reinink Let me know if this can be merged |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jamesst20! Just tested locally and it looks good 👏
Let's just wait for @joetannenbaum and @reinink before we merge it 😉
Could we proceed? It doesn't look like anyone is looking into it? |
If others are required for review before this can be merged I suggest adding them as reviewers on the PR so they will be more likely to be aware that they are blocking if that is the case. |
It appears to me the project Inertia is dead and there are no more reasons for me to keep my work open and maintain it. I am closing all my opened PRs which includes #1864 #1872 #1873 #1874 #1875 #1881 I will be maintaining unofficially my own fork of this library so my projects using Svelte 5 can still live until I find or create an appropriate replacement for this library. Thanks to everyone who spent time looking at my PRs. |
[Svelte] Improve typescript types + minor fixes inertiajs#1881
[Svelte] Improve typescript types + minor fixes inertiajs#1881
@jamesst20 It's really unfortunate to hear that the Inertia project might no longer be actively maintained. I understand why you decided to close your PRs, but it's still a pity, given all the valuable contributions you've made. |
With pleasure :) I'm glad I am not the only one interested into it :) My fork is here https://github.com/jamesst20/inertia/ The main branch is for Svelte 5 and there is also a backup branch that has a Svelte 5 separate adapter. I would recommend you use the main branch since it will be the one I keep up to date but I only completed yesterday. The old branch have months of testing but it's mostly the same but forked from the current typescript implementation instead :) Backup branch : https://www.npmjs.com/package/@jamesst20/inertia-svelte5 Main branch : https://www.npmjs.com/package/@jamesst20/inertia-svelte Also use https://www.npmjs.com/package/@jamesst20/inertia-core |
Bummer dude 😕 We've been working really hard on Inertia.js v2.0 (as you can see in Taylor's Laracon keynote), so the project is far from dead — we're just in a bit of an awkward position between major versions. I apologize that your work has not received more attention, that's partly my fault for not having enough time to review PRs recently. I don't work on Inertia.js full time like other open source maintainers (ie. Laravel, Livewire), so for me it actually requires time out of my personal life. Because of that things just take longer, as much as I wish it didn't. Thanks for your contributions either way. |
@jamesst20, I totally get where you're coming from, and I'm really sorry for the frustration you've been feeling. We can definitely do better here. I haven't been as involved as I wished. I was away from the project for the last two years, but I'm back now and really grateful for everyone who's kept contributing in the meantime 🙇 The team has been heads down working on Inertia.js v2, especially leading up to the Laracon announcement last week. There are a lot of cool new features coming, and I'm excited to be more involved again! Just last Friday I mentioned internally that getting #1881 and #1872 reviewed and approved was my top priority this week since I'm blocked by them too. I'd really appreciate it if you could reopen these two PRs. After that, I'll be focusing on fixing #1670 and adding all the new v2 stuff to the Svelte adapter so we can launch it with the other adapters next month. Thanks again for all your hard work and contributions. It really means a lot! |
I appreciate the clarification, and I apologize for my earlier comment — it seems I misunderstood the situation. I had been following this PR because I need the functionality it provides, and I mistakenly thought the project might not be actively maintained. |
#1963 has been merged and will be tagged in the next release 🥳 |
@reinink
I have rebased all my commits in my repo (https://github.com/jamesst20/inertia) to include the latest changes and afterwards I have taken all my packages/svelte files there to overwrite the ones in the current repo. I have stripped down all unnecessary changes and all new features as well.
This PR only improve the types and fix some possible regression that I will comment in a few seconds in the diff
Edit: @reinink it's ready. i'm done adding comments :)