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

@wagmi/svelte #4365

Open
wants to merge 73 commits into
base: main
Choose a base branch
from
Open

@wagmi/svelte #4365

wants to merge 73 commits into from

Conversation

ByteAtATime
Copy link

@ByteAtATime ByteAtATime commented Oct 25, 2024

This PR is an implementation of reactive Wagmi hooks in Svelte. It implements about half of the hooks, mainly building the foundation for future contributions.

Notes

Reactivity

In order to make return types reactive, I have opted to return a function. It's the function that then returns the value. While it is a little unintuitive, it works perfectly with $derived.by:

const { data, isLoading } = $derived.by(useEnsName(...));

Design consideration: should we use React-style .current instead of returning a function?

Why?

Unfortunately, simply returning a primitive from a function doesn't make it reactive. The workaround is to return an object with a getter:

const reactiveReturn = () => {
  // ... do stuff

  return {
    get count() {
      return count;
    }
  }
}

However, I find this a little ugly when used, especially because it cannot be destructured:

const countObj = reactiveReturn();
countObj.count // repetitive?

Similarly, a similar problem occurs with parameters: if a reactive object is directly passed into a function, it won't be reactive:

let counter = $state(69);
const output = useCounter(counter);
                       // ^ State referenced in its own scope will never update. Did you mean to reference it inside a closure?

Happily, this isn't the only framework that has this problem (SolidJS shares a similar problem). In both frameworks, the workaround is to simply pass in a function as a parameter:

let reactiveAddress = $state("0x...");
$derived.by(useEnsName(() => ({ address: reactiveAddress })));

Design consideration: In the RuneParameters type, I have defined it as (() => T) | T. In other words, it accepts either a reactive function or a non-reactive object. Will this lead to confusion?
Edit: Now we can only pass in a function. This makes it easier to compose types and reduces confusion for users.

Design consideration: should the return types and parameters include the function or not?

  • Including the function -> actual return types/parameters
  • Excluding the function -> easier to be composed in types (currently, you would need to use ReturnType<UseChainIdReturnType> or ReturnType<UseChainIdParameters>)

Testing

So far, I don't see a way of testing the Svelte package without adding the dependencies to the root. For now, I'm simply running vitest in the package instead of adding it to the Vitest workspace -- I will look into it in the future.
Edit: I'm still using a seperate vite.config.ts, but it's added into the workspace

Biome currently only has partial support for Svelte files. It can successfully format the <script> portion, but when linting it doesn't check the HTML part, leading to many variables being marked as unused.
This allows us to accept a function as a parameter, which allows for reactivity (if we pass a raw value, it won't be reactive).
I'm not sure how to get Svelte tests into the Vitest workspace. Without installing a bunch of dependencies into the root folder, it cannot run the Svelte-specific code.
This required a bit more refactoring on the testing system. There are still a few things I haven't figured out yet, but we're getting there :).
Copy link

changeset-bot bot commented Oct 25, 2024

⚠️ No Changeset found

Latest commit: c489000

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

💥 An error occurred when fetching the changed packages and changesets in this PR
Some errors occurred when validating the changesets config:
The package "@wagmi/svelte" depends on the ignored package "@wagmi/test", but "@wagmi/svelte" is not being ignored. Please add "@wagmi/svelte" to the `ignore` option.

Copy link

vercel bot commented Oct 25, 2024

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

Name Status Preview Comments Updated (UTC)
wagmi ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 18, 2024 11:34pm

Copy link

socket-security bot commented Oct 25, 2024

Copy link

socket-security bot commented Oct 25, 2024

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Alert Package NoteSourceCI
Obfuscated code npm/svelte2tsx@0.7.22 ⚠︎

View full report↗︎

Next steps

What is obfuscated code?

Obfuscated files are intentionally packed to hide their behavior. This could be a sign of malware.

Packages should not obfuscate their code. Consider not using packages with obfuscated code

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/foo@1.0.0 or ignore all packages with @SocketSecurity ignore-all

  • @SocketSecurity ignore npm/svelte2tsx@0.7.22

Not only was this a bad choice for users (as they might not know why it's not being reactive), it also turns out to be harder to use in types.
Copy link
Member

@tmm tmm left a comment

Choose a reason for hiding this comment

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

Looking good! Some comments. Let's also move the playground to playgrounds/sveltekit so packages/svelte/src/ follows the same directory structure as the other packages.

packages/svelte/.npmrc Outdated Show resolved Hide resolved
packages/svelte/.gitignore Outdated Show resolved Hide resolved
packages/svelte/README.md Outdated Show resolved Hide resolved
packages/svelte/package.json Show resolved Hide resolved
packages/svelte/src/lib/hooks/test.svelte.ts Outdated Show resolved Hide resolved
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.

2 participants