Skip to content
This repository has been archived by the owner on Apr 6, 2023. It is now read-only.

feat(nuxt): useData composable #7569

Closed
wants to merge 3 commits into from
Closed

feat(nuxt): useData composable #7569

wants to merge 3 commits into from

Conversation

pi0
Copy link
Member

@pi0 pi0 commented Sep 15, 2022

πŸ”— Linked issue

Resolves nuxt/nuxt#14924

❓ 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

This PR introduces a new composable similar to useState but with different characteristics that makes one able to create their own useAsyncData with full customization.

  • It is meant for page-data similar to useAsyncData (instead of initial-data that useState is meant for)
  • Key is always required
  • Init function is not supported as we expect user to have it's own logic outside of composable
  • It is extracted to the payload for full-static support
  • On full-static, it will be replaced by navigation (if multiple values are needed, should use distinct keys)

Notes: Later, we can introduce auto key and actual shared state with useAyncData but it is minimum required to create a distinguished version of useState and can support payload extraction.

πŸ“ Checklist

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

@netlify
Copy link

netlify bot commented Sep 15, 2022

βœ… Deploy Preview for nuxt3-docs canceled.

Name Link
πŸ”¨ Latest commit 719f7cc
πŸ” Latest deploy log https://app.netlify.com/sites/nuxt3-docs/deploys/6324383f890e1200085e90c6

@pi0 pi0 marked this pull request as ready for review September 16, 2022 08:29
@danielroe
Copy link
Member

With global state (this includes useAsyncData, useFetch and useState, which are all currently global and share the same namespace within each composable's key prefix), we have a potential footgun when it comes to <Suspense>.

This issue exists anyway but it becomes particularly apparent when we start extracting page-specific payloads.

Consider the following code in ~/pages/users/[id].vue:

const { data } = useAsyncData(
  () => $fetch(`https://api/users/${route.params.id}`),
  { initialCache: false }
)

// or

const pageData = useData()
if (!pageData.value) {
  pageData.value = await $fetch(`https://api/users/${route.params.id}`)
}

When the payload is loaded for the new page it will automatically trigger a re-render of the current one being displayed, before then resolving suspense and showing the new page. This can result in an unsightly flicker with the updated data being applied to the current page.

The way we've resolved this with route based information is by ensuring that useRoute() returns the correct value in different Suspense forks. (And, outside Suspense, it returns the last loaded page - that is, it doesn't update whilst Suspense is resolving.)

Creating this new useData composable is an opportunity to ensure that page-based data is not global but tied to a particular route, which would ensure that we can prevent flickering when loading new data. (And I would recommend it be renamed usePageData.)

For example, a conceptual implementation:

const usePageData = (key?: string /* automatically injected by magic */) => {
  const route = useRoute()
  const nuxtApp = useNuxtApp()
  // ... initialisation logic omitted for simplicity
  return computed(() => nuxtApp.payload.data._page[route.path])
}

This would ensure that we have a single data cache throughout the app, but also that each suspense fork shows the correct data.

usePageData would be explicitly for data linked to a particular route, and useState could continue to be used for global state that is not linked to a particular route. BothΒ should be extracted to payload, with the exception of useState calls that are not called within a component instance, which can be safely inlined into the page as they are run on 'setup'.

It's important that we make a decision on this before merging this PR as a change to this (to make useData/usePageData locally scoped) would be breaking later down the line.

@pi0
Copy link
Member Author

pi0 commented Sep 16, 2022

As mentioned in #7567 (comment), I don't like to make a different behavior of useState based on the context it is called. It should be always considered global and behave consistently. However, we could include diff between two pages to the payload, more thinking and should keep it as simple as possible without depending on any routing logic. This is the purpose of use*Data composables to be extracted to the (data)payload.

For the rest of the explanation, I fully agree with your reasoning. And honestly, it is the main reason in the initial implementation I made to make key mandatory. Key passed to useData should be unique to the route to avoid overriding. It wouldn't break if we introduce smart automated keys in the feature but also give users full control over key setting strategy. (nor enforcing one)

To be honest, I really love the idea of route scoped usePageData composable too (would be nice to make a new issue and open to have it with what you propose!) but it doesn't replace useData. Both can co-exist finally.

useData is slimmed-down version of useAsyncData and any caching option or breaking changes we introduce will reflect all 3. (On breaking, if you think is important we really need to discuss and do it ASAP directly for useAsyncData before 3.0.0 that's much bigger breaking change surface than introducing useData!)

Copy link
Member

  1. I really do think this is the wrong approach. By extracting data into the payload and specifying that this is 'page-specific' data - but applying it globally - we are making it much easier for users to trigger mismatching Suspense state. That is, we are encouraging it, by suggesting that there can be global but page-specific state. And it will be harder to fix later.

    However, if you don't want to make useData locally scoped, what about at least making the fallback key locally scoped? So if user does not pass a specific key, then a route hash is appended to the key. This would still let users use global data or override the route scoping by passing a string key.

    I realise you want to merge this ASAP but I think it deserves resolving this discussion; I think this composable will become a key part of using Nuxt and I think even though it might be nice to get it in for RC11, if we don't have enough time over the weekend to thrash this out properly, then we should defer it.

  2. The use case for useAsyncData is not anywhere stated to be page-specific, and, at least to me, it's not intuitive that use*Data is about a specific page. In fact, I think there is further discussion to be had regarding the intended use case of useAsyncData in relation to routing...

  3. Similarly, I think we can improve the naming. As is, I think it will cause confusion, because useData is not 'like' useAsyncData; it's 'like' useState. When looked at the names, useData, useAsyncData and useState would (I think) be a fairly confusing trio. Maybe usePageState (if you do decide to locally scope it) or usePayloadState?

  4. A question: what do you mean by saying that any caching option or breaking changes we introduce will reflect all 3? I wasn't proposing making useAsyncData locally scoped but you do raise an interesting point!

No matter what we do, I think it's important we communicate quite clearly what the purpose of these composables is (initial state vs page-specific info vs global state etc.). So far there has been no stated purpose attached to them (of course, it hasn't mattered until now, with payload extraction). Decisions, for good or ill, about global/local/page scope, need to be clearly explained and committed to so projects aren't broken in subsequent releases.

@pi0
Copy link
Member Author

pi0 commented Sep 16, 2022

I think you are confusing too many different topics over this PR.

Extracted payload introduced in RC.10, is technically page data payload. Regardless of what we include in it and by what order or strategy apply updates on client-side navigation. useData enables to use of this namespace and is the minimum required function in order to leverage payload extraction when not using useAsyncData and useFetch. (again, not in contrast to your other idea about page-scoped data such as usePageData).

The scope of useData, should be exactly the same as useAsyncData. And as you mentioned both are not always page-specific but global keys which can be also scoped to the page. It is another topic of how it is to be properly used with payload extraction and it applies to both useAsyncData and useData equally. We can mark it as experimental in the documentation until we try it with some examples so there is no miscommunication :)

On your suggestion about key fallback with route hash, there is no fallback key support at all if you read PR description! The key is explicit. I still believe usePageData is a really nice idea and can do it directly!

All that said, I really see no blocker or potential breaking change for introducing useData (with explicit keys as proposed) any trying it. It is also really not in contrast or replacement for usePageData. But since RC.11 is a hotfix, we can delay to introduce both for RC.12 and try them πŸ‘πŸΌ At the same time will try to improve implementation to be closer to useAsyncData before merging this.

@pi0
Copy link
Member Author

pi0 commented Nov 28, 2022

Closing in favor of #9262

@pi0 pi0 closed this Nov 28, 2022
@danielroe danielroe added the 3.x label Jan 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

useData composable
2 participants