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: add brisa adapter #1913

Closed
wants to merge 3 commits into from
Closed

feat: add brisa adapter #1913

wants to merge 3 commits into from

Conversation

anubra266
Copy link
Collaborator

@anubra266 anubra266 commented Oct 8, 2024

add brisa adapter

related #1912 brisa-build/brisa#529

Copy link

changeset-bot bot commented Oct 8, 2024

⚠️ No Changeset found

Latest commit: 82f06f8

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 "brisa" depends on the ignored package "@zag-js/shared", but "brisa" is not being ignored. Please add "brisa" to the `ignore` option.

Copy link

vercel bot commented Oct 8, 2024

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

Name Status Preview Updated (UTC)
zag-nextjs ✅ Ready (Inspect) Visit Preview Oct 10, 2024 9:22pm
zag-solid ✅ Ready (Inspect) Visit Preview Oct 10, 2024 9:22pm
zag-svelte ✅ Ready (Inspect) Visit Preview Oct 10, 2024 9:22pm
zag-vue ✅ Ready (Inspect) Visit Preview Oct 10, 2024 9:22pm
zag-website ✅ Ready (Inspect) Visit Preview Oct 10, 2024 9:22pm

@anubra266
Copy link
Collaborator Author

anubra266 commented Oct 8, 2024

@osdiab @aralroca I've tried to come up with a v0 for brisa.
There's probably more improvements that can be done on the current approach. An issue I currently face is letting the final api.connect be reactive. I tried using derived but that seems to make the whole component state/tree react when state changes. Resetting component back to initial state.

Adapter is in packages/frameworks/brisa dir,
While example is in exmples/brisa dir

Also, I'd love to know if there's any more JSX differences to watch out for. E.g.

passing data-attr=undefined to react element removes the data-attr, but brisa leaves it with "undefined" string value
passing disabled="false" in react does not disable element, but it does in brisa. I've resolved this two cases, just want to know if there's any other behaviours to normalize

@aralroca
Copy link

aralroca commented Oct 8, 2024

That's cool! 🔥

@aralroca
Copy link

aralroca commented Oct 8, 2024

About the behavior of JSX, it is very much inspired by React, so the difference has to be minimal. As a difference, instead of className is class, and in SVGs you don't have to do the conversion from fill-rule, clip-rule to fillRule or clipRule, already fill-rule or clip-rule (and the rest similar) work.

@segunadebayo segunadebayo marked this pull request as draft October 8, 2024 12:26
@anubra266
Copy link
Collaborator Author

anubra266 commented Oct 9, 2024

@aralroca @osdiab There's a behaviour that seems problematic for the adapter, I'm keeping zag state like this:

 const state = state(service.state)

service is zag's running state.

When service is mutated, it seems brisa state()'s value also updates automatically. So state variable defined gets a new value. Butt, derived wouldn't detect this subtle change. How would I achieve something like a ref. I only want the state I defined to change when I tell it to with something like

const unsubscribe = subscribe(service.state, () => {
   state.value = snapshot(service.state)
 })

@anubra266
Copy link
Collaborator Author

Closing this for now. Will be picked up after problems are clarified

@anubra266 anubra266 closed this Oct 17, 2024
@aralroca
Copy link

I had not seen the question until now.

In Brisa "state" also serves as "ref", and the code you mention should work, although I would not put the variable name the same. This should work:

export default function Component({}, { state, onMount, cleanup }) {
  const zagState = state(service.state)

  onMount(() => {
    const unsubscribe = subscribe(service.state, () =>  {
       zagState.value = snapshot(service.state)
     })
    cleanup(() => unsubscribe())
  })

  return <div>{JSON.parse(zagState.value)}</div>
}

@anubra266
Copy link
Collaborator Author

@aralroca Still wasn't able to get it to work. Would be awesome if you could help look into it. It's the useSnapshot.ts file

@aralroca
Copy link

I think the problem is that in Brisa we have this condition inside the signal setter:

https://github.com/brisa-build/brisa/blob/02286643517d8a6648f064c8e34a3d28b690a341/packages/brisa/src/utils/signals/index.ts#L116

This way if the value is the same we avoid updating the DOM with the same value. I think the problem comes from here if it is mutating without being reactive inside the Zag service.

I don't know how feasible is to do something like this to force reactivity:

const unsubscribe = subscribe(service.state, () => {
  // Create a new object to force reactivity even if the values are the same
  state.value = { ...snapshot(service.state) }; 
});

we were not contemplating that mutations could be made external to the state in this way. If this is a common behavior of several libraries, we can reconsider changing this part.

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