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(nuxt): allow accessing NuxtPage ref via pageRef #19403

Merged
merged 19 commits into from
Jun 10, 2023

Conversation

huang-julien
Copy link
Member

@huang-julien huang-julien commented Mar 2, 2023

πŸ”— Linked issue

resolve #19268

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Hi πŸ‘‹ this PR makes using ref on <NuxtPage> possible.

The idea behind this PR is to set a ref on the page component called within RouteProvider and expose it. Then NuxtPage component will also expose what has been exposed through the RouteProvider's ref.

I did some manuel tests, and didn't find any memory leaks.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@codesandbox
Copy link

codesandbox bot commented Mar 2, 2023

CodeSandbox logoCodeSandbox logoΒ  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

@danielroe danielroe added the 3.x label Mar 11, 2023
@danielroe danielroe requested a review from antfu March 21, 2023 07:40
@antfu
Copy link
Member

antfu commented Mar 22, 2023

I am more curious about what would be the use cases behind it. This to me feels like quite some code added for features that most of ppl don't need. And also be able to work around with a custom inject/provide etc.

Other than that, I also feel the current design is a bit implicit, where the ref gets the child, not the component feels a bit counter-intuitive. This also makes it harder to keep type safety.

@huang-julien
Copy link
Member Author

huang-julien commented Mar 22, 2023

I was planning to try another approach.
Maybe should we do something done by vue core here ? Do you think This can be possible ?
This feature will indeed not be used by most of ppl πŸ€·β€β™‚οΈ , but I think those who wants to use it are expecting to be able to have an acess to refs.
edit: btw if you think we should'nt do it, let's just add a warning within the docs

@antfu
Copy link
Member

antfu commented Mar 22, 2023

I would first challenge the real-world use case of the original request, and see if there are better solutions to achieve what they need (maybe no need to mess with the ref) before discussing the design of API etc.

@JasonGotGithub
Copy link

JasonGotGithub commented Mar 24, 2023

@antfu In essence it's about having more control when make page transitions, whether that would access (animation) tweens, dom elements which or page components. More specially, "control" in terms of a page transition might entail more than solely animating/manipulating the page el. In Nuxt 2 it was possible to do this by accessing the page through a ref via the parent for example. In the documentation there was no mention of this feature being deprecated for Nuxt 3.

edit: I wanted to add a note about "most of ppl don't need". Nuxt is quite popular in the creative development realm of coding (link for reference)

@JasonGotGithub
Copy link

Hey @danielroe @antfu, what information do you need specifically to progress this?

Does an example in Nuxt 2 help? This way you can see the use case in a demo instead of text

@danielroe
Copy link
Member

Does an example in Nuxt 2 help? This way you can see the use case in a demo instead of text

That would be great ❀️

@antfu
Copy link
Member

antfu commented Apr 12, 2023

I think I understand the intention, but the current approach is a bit hacky to me. I guess for this, maybe we could have something built-in to support onBeforeEnter hooks in definePageMeta and compile away at build time.

@JasonGotGithub
Copy link

Cool! I'll make a demo asap just to be sure the use case is clear

@JasonGotGithub
Copy link

Hey @danielroe and @antfu, I've made a quick example. It basically contains two concepts:

  1. Accessing a component which is (almost) always visible on a page. In this case it's the Navigation component. In the example the timeline tween gets triggered exactly when leaving a page. Reason being that visually it looks cohesive. It's possible to trigger the animation with a $route watcher in this case. However, it visually will happen after the page is shown.
  2. Looking at some data from the page component or its child component. In this case I've kept in simple. Based on if the home page timeline tween has progressed past a certain point you either reverse the timeline tween or you pause it and make a simpler tween. In more complex situations you might have a staggered animation of letters from -100% to 0%. If the animation has progress to something like .95, it might be more visually appealing to create a new staggered animation which goes to 100% then to reverse the existing/original staggered animation

Even though I've added two concepts, it's not exhaustive. Another concept/example might be accessing data from an 3DPool (when working with WebGL) and manipulating/animating those before arriving at a new route or finishing the route navigation

@JasonGotGithub
Copy link

JasonGotGithub commented May 19, 2023

Hey @danielroe and @antfu, I wanted check in to see if you have everything you need from me for this and if I can help out in some way

@JasonGotGithub
Copy link

Hi @antfu & @danielroe, is there an update on this?

Copy link
Member

@danielroe danielroe left a comment

Choose a reason for hiding this comment

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

I'm very much up for the concept behind this. If we can expose the exposed properties of the page component 'upwards' that seems good to me. But I am wary of the particular implementation here which is clever, but very involved and I fear might be breakable.

How about exposing a proxy that accesses whatever the page exposes instead? That way we wouldn't need to manipulate vnodes. It would add an extra property name but otherwise pretty simple. Something like:

const pageRef = ref()

// later
pageRef.value.page.someMethod()

Mind you, I haven't tried this. What do you think?

@danielroe
Copy link
Member

Pushed an implementation. Let me know what you think and feel free to update/amend.

@danielroe
Copy link
Member

/trigger release

@nuxt nuxt deleted a comment from github-actions bot Jun 10, 2023
@github-actions
Copy link
Contributor

πŸš€ Release triggered! You can now install nuxt@npm:nuxt3@pr-19403

@danielroe danielroe dismissed their stale review June 10, 2023 17:05

pushed changes

@huang-julien
Copy link
Member Author

The implementation is working well and way simpler than what i did. I like it πŸ‘

huang-julien added a commit to huang-julien/nuxt that referenced this pull request Jun 10, 2023
@danielroe
Copy link
Member

Can you think a better API/name for the exposed property? Maybe pageRef (similar to pageKey) and layoutRef directly?

@huang-julien
Copy link
Member Author

huang-julien commented Jun 10, 2023

Sure ! This would be less confusing (pageRef and layoutRef)

@danielroe
Copy link
Member

Just one more idea - might we pass page-ref as a prop? So then it removes double-level access?

@huang-julien
Copy link
Member Author

huang-julien commented Jun 10, 2023

I think the tempalte transformation would transform :page-ref="page" in to :page-ref="page.value" when a ref need the full ref() object. But it would be nice if that work

@danielroe danielroe changed the title feat(nuxt): forward page ref to NuxtPage feat(nuxt): allow accessing NuxtPage ref via pageRef Jun 10, 2023
@danielroe danielroe merged commit 319935f into nuxt:main Jun 10, 2023
23 checks passed
@github-actions github-actions bot mentioned this pull request Jun 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DefineExpose not working for page components. Transition property causing issues with console log
4 participants