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 svelte portable #390

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

Conversation

DaniAcu
Copy link
Contributor

@DaniAcu DaniAcu commented Oct 6, 2024

Hi I migrate lenis-svelte to this repo.

The approuch for svelte is using svelte action and svelte stores, things that will work and still working for Svelte 4 and Svelte 5. Despite the changes to runes in Svelte 5 the svelte will still working in that version.

@DaniAcu DaniAcu requested a review from a team as a code owner October 6, 2024 17:57
@DaniAcu DaniAcu requested review from arzafran and GFier October 6, 2024 17:57
Copy link

vercel bot commented Oct 6, 2024

@DaniAcu is attempting to deploy a commit to the darkroom Team on Vercel.

A member of the Team first needs to authorize it.

@clementroche clementroche requested a review from feledori October 6, 2024 19:06
@feledori
Copy link
Contributor

feledori commented Oct 7, 2024

hello, first of thanks for contributing! we were thinking about adding lenis/svelte and waiting for svelte 5, which should be right around the corner. not to keen on supporting svelte version below 5 as it creates overhead. but open for opinions as I am not to deep in the svelte community. my thought is starting of with svelte 5 so we just have to maintain versions beyond 5, which afaik is the big shift for svelte.

@DaniAcu
Copy link
Contributor Author

DaniAcu commented Oct 7, 2024

Hi! Thanks, related to the svelte migration, there a lot of noice around it that doesn't make too much sense.

Svelte 4 will be compatible with Svelte 5, at least in the features that we are using here (store and actions). Props component and other stuff will change, but it will not break. It will be removed in future versions (svelte 7)

Here the docs: https://svelte-5-preview.vercel.app/docs/faq#breaking-changes-and-migration-no-but-really-am-i-going-to-have-to-rewrite-everything

There no plan for deprecate stores or actions, the motion library svelte/motion has stores an plan to continue having it for a while. https://github.com/sveltejs/svelte/blob/main/packages/svelte/src/motion/spring.js#L62

Implementing a portal with only runes, it will limit the use of the library to apps that needs migration. While store are still usefull, combined with action create a simple portal that will work in Svelte 5 without a change.

I make that choice to support the stable version, and the RC. We don't need to choose right now which will be the better option, because with the combination of stores and action we have a stable a robust solution for old versions and new one.

@clementroche
Copy link
Member

clementroche commented Nov 12, 2024

yo @DaniAcu it seems that Svelte5 has been released, can you confirm everything is working great? Also we've made some changes about raf, it's much simpler now and happens at lenis/core level, can you update the lib accordingly? you might just need to remove everything related to raf within lenis/svelte.

@DaniAcu
Copy link
Contributor Author

DaniAcu commented Nov 13, 2024

Hi! I tested with svelte 5 and works perfectly. I will check what you mention about raf, and remove it.

After merging, I cloud track the work of moving it to runes, for when svelte remove stores. But not official news about it yet. They still having stores for svelte/motion.

@DaniAcu
Copy link
Contributor Author

DaniAcu commented Nov 14, 2024

I already test it with svelte 5 and is working right. I updated now that the autoRaf is not necessary here.

Let me know if you have any question.

@clementroche
Copy link
Member

clementroche commented Nov 14, 2024

Reviewing the README and i'm wondering why you're not using a custom component such as <Lenis /> (see React example). Also having to use id to acces to lenis instance into sub components looks really weird to me, i don't see the benefits over using vanilla lenis to be honest, it looks overly complicated.

I think this implementation should look like lenis/react as much as possible, with a <Lenis> component and a useLenis hook

@DaniAcu
Copy link
Contributor Author

DaniAcu commented Nov 15, 2024

I cloud adapt to have similar apis as vue and react.

The idea here about the instance by id, it was to have a clear identification or knows what lenis instance you are refering of using when you are using it.

In the current react abstraction, we are not beeing explicit about it. Any useLenis, cloud mean any instace in a project. In cases of sub instances, you will always needs to check if the component that use the hook is inside of a wrapper and in which others wrappers is wrapped. Also the behavior related to root and any other instance is different, I mean in the same component you cloud use root and useLenis, but for nested instance you need another child component.

From my point of view any instance should be accessed in the same way. Like 1 API to rules them all. I like the idea to have instances creator and the instance getter to the same level.

So the main idea behind id and this abstraction is:

  1. Give a possibility to get the lenis root instance in a component inside of a lenis region. (currently impossible in react abstraction)
  2. Make possible have the lenis creator and setter and the same level doesn't matter if is a root instance or not.

Maybe there things that I miss in this consideration. Is it make sense sense use a root instance when you are in the lenis wrapper? Maybe is a thing that you want to prevent in the use of the library, and idk.

I just share here, how I think about. Let me know what do you think, I cloud make the refactor after that 😊

@clementroche
Copy link
Member

Hey @DaniAcu, your questions are absolutely valid, and I appreciate the thought you've put into this. That said, in my experience using lenis/react for the past two years, I’ve never encountered a situation where accessing a specific Lenis instance via IDs was necessary. From what I’ve seen, neither have other users raised this as a concern.

One of the biggest strengths of lenis/react is its simplicity: just relying on the inherited Lenis instance works seamlessly for the majority of use cases. The API you’ve proposed (e.g., useLenis('root', () => {}) with optional ID selectors) does add functionality, but at the cost of complexity. I’m concerned it might make lenis/svelte harder to understand while offering relatively limited value in return.

I truly believe that consistency across frameworks (lenis/react, lenis/vue, and now lenis/svelte) is key to providing a clear and intuitive experience for users. Any deviations between them could create unnecessary confusion. To that end, I think lenis/svelte should stick to the same approach as the other frameworks for now.

That said, the feature you’re proposing could absolutely be revisited later, potentially as a breaking change in the future. For example, we could introduce an API like useLenis('root', () => {}) as an optional enhancement if there’s strong demand for it.

Looking ahead, we could consider a more generic implementation that’s flexible across all frameworks (react, vue, svelte). This would not only keep things consistent but also make maintenance easier while allowing room for additional features.

Let me know what you think—happy to collaborate on a solution that balances simplicity, consistency, and extensibility!

@DaniAcu
Copy link
Contributor Author

DaniAcu commented Nov 15, 2024

Yeah, the id is something that I don't like from the svelte abstraction. Let me work in the refactor to have the same, maybe in the time while I refactor this comes to my mind some other idea, specially to make root and sub instances beeing used with the same approach.

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