Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

refactor: change primitive to shallowRef, rename ref usage to deepRef #4582

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

Merged
merged 6 commits into from
Feb 15, 2025

Conversation

antfu
Copy link
Member

@antfu antfu commented Feb 14, 2025

Part of #4575

This PR is a refactor, should not introduce any behavioral change

  • change primitive refs like ref(0) to shallowRef(0)
  • rename other ref() usage to deepRef() (alias from ref()) for better explicitness
  • local ESLint rule to restrict ref() usage

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Feb 14, 2025
@antfu antfu changed the title refactor: change primitive ref to shallowRef refactor: change primitive shallowRef, rename ref usage to deepRef Feb 14, 2025
@antfu antfu changed the title refactor: change primitive shallowRef, rename ref usage to deepRef refactor: change primitive to shallowRef, rename ref usage to deepRef Feb 14, 2025
@TheAlexLichter
Copy link

Some personal thoughts on the PR & Ideas:

  1. I like that it improves consistency across the codebase and should help understanding the difference between shallow and "normal" (deep) refs
  2. Could it become more confusing for new contributors to look through the codebase as it strays away from the classic Vue naming convention?
  3. Besides inconsistencies, this should improve perf (I assume) - but does it actually? Is it quantifiable or more of a micro optimization? I suppose with one (if not the) most used Vue 3 library, this could definitely benefit some applications but I'd be curious when that point is reached/noticable for a few of the changed composables.

@antfu
Copy link
Member Author

antfu commented Feb 14, 2025

  1. With the ESLint rule, contributors should get the error with hint to choose between deep and shallow - this brings the awareness of what ref you are using.
  2. This PR itself does not improve or change the performance (there should be follow-up PRs that changes some deepRef to shallowRef, which would actually improve the performance, but that would be breaking and we need to handle it carefully). But more like a move that we are trying to make the shallow default. You can argue that most of the time performance is imperceptible even on nested objects/arrays (as long as they are not crazily big). Micro or not, I consider this not an "optimization", but more like a default performant practice that should be the default, where optimization is not even relevant.

@TheAlexLichter
Copy link

Thanks for the insights @antfu!

Do you think the move to shallowRef by default (similar to how other frameworks do it) would be one that should be adopted by the whole ecosystem, so also for "end users"? If yes, why that? 🤔

@antfu
Copy link
Member Author

antfu commented Feb 14, 2025

I don't honestly sure what would be the best practice for this. Theoretically, yes, I think ref should be shallow by default and ideally the ecosystem should move on that, but the status quote has a strong inertia, where benefits might not always justify the cost of migration. I think we still need time to try and feel about it, there is no final answer yet. But I think VueUse could a good place to start and explore.

@@ -12,7 +12,7 @@ export const OnLongPress = /* #__PURE__ */ defineComponent<OnLongPressProps>({
props: ['as', 'options'] as unknown as undefined,
emits: ['trigger'],
setup(props, { slots, emit }) {
const target = ref()
const target = deepRef()
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be a shallowRef too? since its a ref to an element afaict

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I was using a simple regexp to replace those simple ones to shallow, I didn't go through these details (requires context) in this pr. Definitely something for the future.

Copy link
Collaborator

@43081j 43081j 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 to me

@antfu there's various places where we use a deepRef but possibly shouldn't. i'll ping you on discord and, if im right, will open separate PRs 👍

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Feb 14, 2025
@antfu antfu merged commit d9352a3 into main Feb 15, 2025
8 checks passed
@antfu antfu deleted the refactor/shallowRef branch February 15, 2025 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.