Skip to content

Conversation

austincrim
Copy link
Contributor

Tested with SvelteKit demo app, which includes static and server-side rendered pages, API routes, and form actions.

@google-cla
Copy link

google-cla bot commented Dec 20, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.


// https://github.com/jthegedus/svelte-adapter-firebase/blob/main/src/files/firebase-to-svelte-kit.js
function toSvelteKitRequest(request: Request) {
// Firebase sometimes omits the protocol used. Default to http.
Copy link
Collaborator

@jamesdaniels jamesdaniels Jan 3, 2023

Choose a reason for hiding this comment

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

gen2 functions should handle x-forwarded correctly, so these lines shouldn't be needed any longer—give it a spin in prod though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I can tell, this seems to be on the only way to get the actual full host to pass as the base in the URL constructor that follows. I couldn't find a way to piece together the host without using these headers.

@TheIronDev
Copy link
Contributor

Can you write tests for the 3 functions you are introducing?

@austincrim
Copy link
Contributor Author

The deployed demo app is not totally functional yet. The Wordle clone doesn't work, I think due to some issues with cookies. It works fine locally, just seeing issues when deployed. I'm investigating further.

@jamesdaniels
Copy link
Collaborator

@austincrim austincrim marked this pull request as ready for review February 27, 2023 15:24
Copy link
Collaborator

@jamesdaniels jamesdaniels left a comment

Choose a reason for hiding this comment

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

Fix the build error in the tests, think we need to bump the Vite dep to 4

@jamesdaniels jamesdaniels merged commit aa62d56 into FirebaseExtended:main Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants