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

refactor(vue): optimize reactivity handling and reduce bundle size #929

Merged
merged 3 commits into from
Jul 12, 2024

Conversation

negezor
Copy link
Contributor

@negezor negezor commented Jul 3, 2024

Hello, I accidentally came across your module and, after looking at the source code, I really liked it. I would like to thank you for it.

While examining the Vue implementation, I noticed a few areas for improvement. Here is the list:

  1. Use shallowRef instead of ref. It is standard practice to use it for HTMLElement and heavy objects that do not require reactivity. We just need to observe the value setting in this case.
  2. Use simple variables to store storedOptions and storedPlugins that do not need reactivity. After all, we only add watch for Ref objects.
  3. The first two improvements also slightly reduce the minified bundle size, as we only call isRef once and remove unnecessary accesses to .value.

@davidjerleke
Copy link
Owner

Thanks for your contribution @negezor.

  1. Use shallowRef instead of ref. It is standard practice to use it for HTMLElement and heavy objects that do not require reactivity. We just need to observe the value setting in this case.

Thanks. Makes sense 👍.

  1. Use simple variables to store storedOptions and storedPlugins that do not need reactivity. After all, we only add watch for Ref objects.

I'm not following here. Options and plugins should be reactive. Whenever options or plugins change, the carousel should re-initialize. Or am I misunderstanding what you mean by this?

  1. The first two improvements also slightly reduce the minified bundle size, as we only call isRef once and remove unnecessary accesses to .value.

Thanks!

@davidjerleke davidjerleke added the vue Issue is related to Vue label Jul 6, 2024
@negezor
Copy link
Contributor Author

negezor commented Jul 6, 2024

I'm not following here. Options and plugins should be reactive. Whenever options or plugins change, the carousel should re-initialize. Or am I misunderstanding what you mean by this?

Options and plugins will be reactive. We don't just need to store them in a reactive variable, because we only listen for changes to the passed arguments.

@davidjerleke
Copy link
Owner

davidjerleke commented Jul 7, 2024

Options and plugins will be reactive. We don't just need to store them in a reactive variable, because we only listen for changes to the passed arguments.

@negezor thanks for clarifying. I think this is a nice refactor of the Vue wrapper so great work 👍🏻. Have you tested the wrapper with your changes?

@meirroth and/or @sadeghbarati would you like to skim the code and add anything?

@negezor
Copy link
Contributor Author

negezor commented Jul 7, 2024

@negezor thanks for clarifying. I think this is a nice refactor of the Vue wrapper so great work 👍🏻. Have you tested the wrapper with your changes?

@davidjerleke yes. I used the fork of version 8.1.5 with these changes on my stage, as well as later on in production.

@sadeghbarati
Copy link
Contributor

Using shallowRef is LGTM 👌

@meirroth
Copy link
Contributor

Looks good, thanks 👍

@davidjerleke
Copy link
Owner

Just tag me here when this is approved @meirroth and @sadeghbarati. Thanks 🙏🏻.

@sadeghbarati
Copy link
Contributor

@davidjerleke done 😄

@davidjerleke
Copy link
Owner

davidjerleke commented Jul 11, 2024

Thanks everyone for your improvements/contributions and taking time to review this PR 🎉. I will merge it soon.

I’ll make sure to drop a comment here when it has been released in v8.1.7.

Copy link
Contributor

@meirroth meirroth left a comment

Choose a reason for hiding this comment

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

Looks good 👍 Tested in my project and works fine.

@davidjerleke davidjerleke merged commit 910a8dc into davidjerleke:master Jul 12, 2024
@davidjerleke
Copy link
Owner

Will be included in the v8.1.7 release.

@davidjerleke
Copy link
Owner

@negezor, @sadeghbarati, @meirroth these improvements were just included in the v8.1.7 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
resolved This issue is resolved vue Issue is related to Vue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants