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

feat: svelte 5 #83

Merged
merged 28 commits into from
Oct 30, 2024
Merged

feat: svelte 5 #83

merged 28 commits into from
Oct 30, 2024

Conversation

PuruVJ
Copy link
Owner

@PuruVJ PuruVJ commented May 1, 2024

Originally intended to just see how fast non-runic svelte 5 would be, I couldn't resist and converted the whole app to svelte 5 with runes. No more stores here, just $state

Copy link

changeset-bot bot commented May 1, 2024

⚠️ No Changeset found

Latest commit: c90ad9a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented May 1, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
macos-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 30, 2024 8:35am

.prettierrc Outdated
{
"files": "*.svelte",
"options": {
"plugins": ["prettier-plugin-svelte"]
Copy link

@benmccann benmccann May 3, 2024

Choose a reason for hiding this comment

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

If I create a new SvelteKit project with create-svelte then plugins is at the top level and I also have a parser override. I'm not sure which is more correct 😆 But I'm surprised they're different

Copy link
Owner Author

Choose a reason for hiding this comment

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

Oh I didn't know about that, I just thought of adding a plugin for just .svelte files. if the global plugin works, its fine ig 😅

Choose a reason for hiding this comment

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

I'd be curious if one is better for performance or if there is a best practice. Like is yours fast because it applies only to svelte files or is the create-svelte one faster because it just has to be instantiated once globally. The more surprising part is that you were able to get away with out the parser setting.

For reference, here's what create-svelte does:

	"plugins": ["prettier-plugin-svelte"],
	"overrides": [{ "files": "*.svelte", "options": { "parser": "svelte" } }]

Copy link
Owner Author

Choose a reason for hiding this comment

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

That's weird, parser svelte shouldn't be needed, I have never added that to any prettierrc file until now.

I tested with pnpx prettier . --write to see if maybe parser svelte is required, but no, it works just as well

@PuruVJ PuruVJ changed the title feat: non-runic svelte 5 feat: svelte 5 May 4, 2024
primaryColor: keyof typeof colors;
};

export const preferences = persisted('macos:preferences', {

Choose a reason for hiding this comment

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

I feel like trying to abstract persisted here isn't doing you any favors because you have to do things like preferences.value.theme.scheme instead of preferences.theme.scheme. Perhaps you can just create the $effect to keep local storage updated directly in this file instead? I think then you don't need the .value since you wouldn't be crossing boundaries to access the persisted value

Copy link
Owner Author

Choose a reason for hiding this comment

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

I kept it like that cuz Joshnuss is working on svelte-persisted-rune, and once that's out I'm sure he'll figure out a way to do .value on primitives and flattening on objects, so I'll upgrade to that then. Not too worried about the syntax atm

Choose a reason for hiding this comment

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

I think preferences.value is fine — better than a) not having something encapsulated and reusable or b) inconsistency. Unless you were to enforce persisted values being non-primitive

Choose a reason for hiding this comment

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

I don't know if there's a way around preferences.value if you want to have it in a separate file even if you restrict it to being an object? Maybe you could use proxies or something?

You can have a decent amount of reuse if you write it as a hook that the consumer needs to install in an $effect. That doesn't buy you much here because this example is so simple, but is potentially something you could do in a more complicated example

Copy link
Owner Author

Choose a reason for hiding this comment

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

I figured it out

import { auto_destroy_effect_root } from './auto-destroy-effect-root.svelte.ts';

type Primitive = string | null | symbol | boolean | number | undefined | bigint;

const is_primitive = (val: any): val is Primitive => {
  return val !== Object(val);
};

export function persisted<T>(key: string, initial: T) {
  const existing = localStorage.getItem(key);

  const primitive = is_primitive(initial);
  const parsed_value = existing ? JSON.parse(existing) : initial;

  let state = $state<T extends Primitive ? { value: T } : T>(
    primitive ? { value: parsed_value } : parsed_value,
  );

  auto_destroy_effect_root(() => {
    $effect(() => {
      // @ts-ignore
      localStorage.setItem(key, JSON.stringify(primitive ? state.value : state));
    });
  });

  return state;
}

Copy link
Owner Author

Choose a reason for hiding this comment

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

Check the commit just after this comment box

Copy link
Owner Author

Choose a reason for hiding this comment

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

Maybe you could use proxies or something?

No need when svelte does it for ya 😜

Choose a reason for hiding this comment

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

Nice. That looks much better!

Choose a reason for hiding this comment

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

Personally don't love the inconsistency but horses for courses!

Tangential to this convo, but: I'm personally of the view that a localStorage abstraction must listen for storage events and synchronise between tabs, otherwise one will clobber the other silently, leading to all sorts of fucked up bugs

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yet to add that, will do so. I'm also planning on upstreaming the changes to svelte-perisisted-store.

@PuruVJ PuruVJ merged commit dffd365 into main Oct 30, 2024
2 checks passed
@PuruVJ PuruVJ deleted the feat/non-rune-svelte-5 branch October 30, 2024 10:01
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.

3 participants