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: add @remix-run/google-cloud-functions package #3397

Closed
wants to merge 11 commits into from

Conversation

penx
Copy link
Contributor

@penx penx commented Jun 6, 2022

Required adapter in order to provide example of deploying to Firebase functions

Closes: #125
Closes: #211

  • Docs
  • Tests

Testing Strategy:

  1. Added unit tests
  2. Created a branch that uses this adapter for all integration tests

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jun 6, 2022

Hi @penx,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run.

Thanks!

- The Remix team

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jun 6, 2022

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@@ -82,7 +83,7 @@
"eslint-plugin-markdown": "^2.2.1",
"eslint-plugin-prefer-let": "^3.0.1",
"express": "^4.17.1",
"jest": "^27.5.1",
"jest": "^28.1.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is required in order to

import { getTestServer } from "@google-cloud/functions-framework/testing";

https://github.com/penx/remix/blob/29be5a6c90e9d1a02db06d6e9f306947ef89d452/packages/remix-google-cloud-functions/__tests__/server-test.ts#L10-L11

Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate why this is necessary?

I don't see any indication in @google-cloud/functions-framework why jest@^28.0.0 is necessary.

Copy link
Contributor Author

@penx penx Jun 6, 2022

Choose a reason for hiding this comment

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

import { getTestServer } from "@google-cloud/functions-framework/testing";

This import makes use of package exports:

https://github.com/GoogleCloudPlatform/functions-framework-nodejs/blob/759aac18ad297363c4651efcf19ab2bf8fa9625a/package.json#L13

Full support for this was added in Jest 28.

In Jest 27, server-test.ts will fail saying it cannot find @google-cloud/functions-framework/testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll open up a PR for the Jest upgrade shortly, which will need to be merged before this one. I can remove the commits from this branch too.

Copy link
Member

Choose a reason for hiding this comment

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

Let's do this in a separate PR instead, so we keep this PR focussed.

I think the Jest 28 change is also causing some tests to fail, so we could solve them in isolation in the new PR.

@MichaelDeBoey MichaelDeBoey changed the title feat(packages/remix-google-cloud-functions): add google cloud functions adapter feat: add @remix-run/google-cloud-functions package Jun 6, 2022
@MichaelDeBoey
Copy link
Member

@penx This seems to be a duplicate of #2266?
Could you please elaborate on the difference?

@MichaelDeBoey MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label Jun 6, 2022
@penx
Copy link
Contributor Author

penx commented Jun 6, 2022

@penx This seems to be a duplicate of #2266?
Could you please elaborate on the difference?

@MichaelDeBoey sure, I was working on this independently and was going to name it @remix-run/firebase as per #125 before realising a more generic cloud-functions was probably a better route given this is what Firebase functions are running on. As such I hadn't seen the above PR until earlier today.

It appears #2266 has the same purpose but with a slightly different approach.

I would suggest using my approach here for various reasons:

Implementation

rawBody

#2266 has the following code which I think is the main difference:

/**
  * Google Cloud Functions includes middleware that processes incoming requests based on their headers and sets the request body to
  * a javascript object. But Remix doesn't like that, remix has its own request processing logic, so we have to turn the body back into
  * something that Remix is expecting. In this case a Buffer with URLSearchParams encoded data works.
  *
  * @param req the request passed in from the Google Cloud Function
  */
 function createRemixBody(req: GcfRequest) {
   let s = new Readable()
   s.push((new URLSearchParams(req.body).toString()).toString());
   s.push(null);
   return s;
 }

https://github.com/remix-run/remix/pull/2266/files#diff-c7fe10f958a1c448a6f62cb813a2868c7b075f470304598ca79e26eb167f018cR141-R153

We don't need to create a new buffer on each request as rawBody is available as a property on req that provides this.

Here's the equivalent code in my version:

  if (req.method !== "GET" && req.method !== "HEAD") {
    init.body = req.rawBody;
  }

https://github.com/penx/remix/blob/29be5a6c90e9d1a02db06d6e9f306947ef89d452/packages/remix-google-cloud-functions/server.ts#L94-L96

This comes from @ryanflorence 's recommendation on Discord and gist

Types

I've used the official types from @google-cloud/functions-framework and avoided referencing Express directly due to subtle differences such as body + rawBody. This should help identify issues if the official types move further away from Express in the future. (edit: #2266 uses these types)

Testing

Naming

  • This adapter works with both google cloud functions v1 and v2. I expect #2266 does too, but I think there's no need to include gen2 in the adapter name

@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label Jun 6, 2022
@penx penx requested a review from MichaelDeBoey June 6, 2022 14:14
@MichaelDeBoey
Copy link
Member

@penx I suggest you to join forces with @eastlondoner on #2266, since he started first on this idea.
You can add suggestions on his PR and/or create a PR against his branch (on his repo).

This way we can focus on keeping all related discussions on the same PR.

@MichaelDeBoey MichaelDeBoey added the duplicate This issue or pull request already exists label Jun 6, 2022
@penx
Copy link
Contributor Author

penx commented Jun 6, 2022

I've left feedback on #2266 and opened eastlondoner#1 with the unit tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed duplicate This issue or pull request already exists
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants