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

fix: type event.$fetch and event.fetch #1343

Merged
merged 11 commits into from
Jun 28, 2023
Merged

fix: type event.$fetch and event.fetch #1343

merged 11 commits into from
Jun 28, 2023

Conversation

danielroe
Copy link
Member

πŸ”— Linked issue

❓ 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)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This updates the typing of event.$fetch to match the ambient $fetch. It should be paired with unjs/h3#414 as the nitro build won't pass until we have this merged upstream.

πŸ“ Checklist

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

@danielroe danielroe added the bug Something isn't working label Jun 23, 2023
@danielroe danielroe requested a review from pi0 June 23, 2023 23:47
@danielroe danielroe self-assigned this Jun 23, 2023
@danielroe
Copy link
Member Author

an odd issue here of circular types between runtime/build and stubs (not related to pr). will look into it more later

@danielroe
Copy link
Member Author

This is pending merge and release of unjs/h3#414.

@danielroe danielroe marked this pull request as draft June 28, 2023 10:33
@pi0 pi0 marked this pull request as ready for review June 28, 2023 11:02
@codecov
Copy link

codecov bot commented Jun 28, 2023

Codecov Report

Merging #1343 (d52768c) into main (373f34c) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1343      +/-   ##
==========================================
+ Coverage   76.58%   76.61%   +0.02%     
==========================================
  Files          70       71       +1     
  Lines        7193     7232      +39     
  Branches      715      718       +3     
==========================================
+ Hits         5509     5541      +32     
- Misses       1683     1690       +7     
  Partials        1        1              
Impacted Files Coverage Ξ”
src/types/h3.ts 100.00% <100.00%> (ΓΈ)
src/types/index.ts 100.00% <100.00%> (ΓΈ)

... and 1 file with indirect coverage changes

@pi0 pi0 changed the title fix: type event.$fetch as $Fetch fix: type event.$fetch and event.fetch Jun 28, 2023
Comment on lines +1 to +19
import type { NitroFetchRequest, $Fetch } from "./fetch";

export type H3EventFetch = (
request: NitroFetchRequest,
init?: RequestInit
) => Promise<Response>;

export type H3Event$Fetch = $Fetch<unknown, NitroFetchRequest>;

declare module "h3" {
interface H3Event {
/** @experimental Calls fetch with same context and request headers */
fetch: H3EventFetch;
/** @experimental Calls fetch with same context and request headers */
$fetch: H3Event$Fetch;
}
}

export {};
Copy link
Member Author

Choose a reason for hiding this comment

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

Locating these types here will not result in augmenting H3Event in a Nuxt/Nitro project

Copy link
Member

Choose a reason for hiding this comment

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

Testing build archive against nitro-deploys, seems to be working fine:

image image

@pi0
Copy link
Member

pi0 commented Jun 28, 2023

Merging to double check againt nuxt with edge too to fix any possible bundling issues before...

@pi0 pi0 merged commit e3f817e into main Jun 28, 2023
@pi0 pi0 deleted the fix/event-fetch-type branch June 28, 2023 12:54
@danielroe
Copy link
Member Author

danielroe commented Jun 28, 2023

Yes, I was checking with build archive as well against a Nuxt project.

@pi0
Copy link
Member

pi0 commented Jun 28, 2023

Against nuxt app (with server/tsconfig.json (seems we are good)

image

@danielroe
Copy link
Member Author

Working for me too - great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants