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(types): add asyncData return types to component instance type #9239

Merged
merged 5 commits into from Jun 2, 2021

Conversation

carbotaniuman
Copy link

@carbotaniuman carbotaniuman commented May 4, 2021

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

This resolves the closed issues #7884 and nuxt/typescript#48. using a variant of @odanado work here. This allows TypeScript users to use the return value of asyncData directly in their computed and methods without having to duplicate things again in data.

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly. (PR: #)
  • I have added tests to cover my changes (if not applicable, please state why)
  • All new and existing tests are passing.
    I'm not sure where to add these kind of tests, or even if we have tests on the type definitions themselves.

@Atinux Atinux requested a review from danielroe May 5, 2021 12:22
@danielroe danielroe changed the title Merge asyncData types in TypeScript feat(types): add asyncData return types to component instance type May 5, 2021
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.

Looks good to me but I'd love @kevinmarrec's eyes on it too.

@kevinmarrec
Copy link
Contributor

kevinmarrec commented May 14, 2021

Thanks for the PR @carbotaniuman

@danielroe I didn't find time yet to check everything cause it needs to perfectly know Vue types to see if there's any mistake.

Also, I thought fetch was used over asyncData since some versions now ? But it may differ depending on user needs.

Finally, before merge I'd like something that show/prove me everything still work (no types breaking) on medium to big projects with these changes.

@Radiergummi
Copy link

Radiergummi commented May 14, 2021

Also, I thought fetch was used over asyncData since some versions now ? But it may differ depending on user needs.

They do slightly different things - asyncData blocks rendering (there's a bit of irony in that) and allows redirecting before finishing the navigation, while fetch is invoked in a component context, so navigation is already complete. Our use case is SEO-sensitive landing pages depending on dynamic URI values that need to render or 301-redirect.

@carbotaniuman
Copy link
Author

Good point. This would probably be better when Nuxt 3 comes around.

@danielroe danielroe reopened this May 31, 2021
@codecov-commenter
Copy link

codecov-commenter commented May 31, 2021

Codecov Report

Merging #9239 (6940911) into dev (1676b86) will decrease coverage by 0.45%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #9239      +/-   ##
==========================================
- Coverage   65.10%   64.65%   -0.46%     
==========================================
  Files          94       94              
  Lines        4098     4215     +117     
  Branches     1121     1168      +47     
==========================================
+ Hits         2668     2725      +57     
- Misses       1152     1191      +39     
- Partials      278      299      +21     
Flag Coverage Δ
unittests 64.65% <ø> (-0.46%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/webpack/src/config/base.js 65.51% <0.00%> (-0.19%) ⬇️
packages/server/src/listener.js 100.00% <0.00%> (ø)
packages/cli/src/utils/generate.js 0.00% <0.00%> (ø)
packages/babel-preset-app/src/index.js 0.00% <0.00%> (ø)
packages/config/src/load.js 75.86% <0.00%> (+1.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1676b86...6940911. Read the comment docs.

@pi0 pi0 merged commit e8281dd into nuxt:dev Jun 2, 2021
@pi0 pi0 mentioned this pull request Aug 11, 2021
@danielroe danielroe added the 2.x label Jan 18, 2023
@danielroe danielroe mentioned this pull request Jan 19, 2023
@danielroe danielroe mentioned this pull request Feb 1, 2023
@rchl
Copy link

rchl commented Mar 9, 2023

Can you please check the follow up at #19526? This seems to cause errors (at least with Vue 2.7, haven't checked 2.6).

@rchl
Copy link

rchl commented Mar 9, 2023

Also this change doesn't even work. At least with vue 2.7:

Screenshot 2023-03-09 at 08 48 44

@rchl
Copy link

rchl commented Mar 9, 2023

It does indeed work with Vue 2.6. But Nuxt 2 comes with Vue 2.7 by default now.

Screenshot 2023-03-09 at 08 51 37

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants