feat(remix-architect): support API gateway v1 #3173
Conversation
|
Hi @wingleung, 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 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 |
|
Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳 |
MichaelDeBoey
changed the title
…teway-v1 # Conflicts: # packages/remix-architect/__tests__/server-test.ts # packages/remix-architect/server.ts
|
sweet! from first glance it looks good to me - I'll take a more in depth look in a few minutes :) |
|
do you have an example repo of this? I'm not having much luck using arc.codes with it, but i could be doing something wrong. |
Thanks for reviewing @mcansh ! I was a bit behind on dev so this PR was indeed broken, my bad. I updated this PR again, I don't use architect but I've tested it with the grunge stack which uses architect and I have it running here in a separate temp branch 👉 wingleung/pull/1 could you try again? sidenote, we use sam cli as local dev tool and a custom pipeline on jenkins to deploy our stack. |
Co-authored-by: Michaël De Boey <info@michaeldeboey.be>
Co-authored-by: Michaël De Boey <info@michaeldeboey.be>
Co-authored-by: Michaël De Boey <info@michaeldeboey.be>
Co-authored-by: Michaël De Boey <info@michaeldeboey.be>
Co-authored-by: Michaël De Boey <info@michaeldeboey.be>
…teway-v1 # Conflicts: # packages/remix-architect/server.ts
…teway-v1 # Conflicts: # packages/remix-architect/magicExports/remix.ts
|
@mcansh any idea when this one can be merge? I'm using a custom build now but it's getting a little harder to keep everything up to date 😅 let me know if you want more info |
|
sorry @wingleung! this keeps getting lost in my sea of tabs - gonna give it another look over and re-verify it's something we want first party support for. I've also haven't been able to run an example thus far, but I'll try again today |
|
Appreciate the work here. It's difficult for us to know all of the different deployment targets well and don't want to be in people's way like this. I personally barely knew enough to make this adapter in the first place and to now support two versions of API Gateway is stretching us even farther. We had to ship with a few "first party" adapters ourselves (because who cares about a new javascript framework?) but the design of Remix is that you can make your own adapter. Long term the adapters should be maintained by the community and the hosts. What if you made your own API Gateway adapter instead? Then more people can deploy to it w/o needing Architect? |
|
@ryanflorence thanks for the insight on the long term strategy here, I definitely understand you want to focus on the core of Remix. And I have no problem splitting this into a separate community project in a leaner form without architect or move it into an keep you posted! |
Let me know how you get along with this! I'd be happy to chip in. Def an interesting problem, especially since there's arguably three relevant payload and response formats: API Gateway v1 & v2, as well as ALB. I've heard rumors that there might be a new streaming response format too, which would also justify a seperate implementation. (Plus I find it personally confusing the package is named architect when it's technically compatible with Serverless, SST, or any other AWS Lambda framework.) |
@RichiCoder1 We have an open issue to support ALB response format by @parisholley for this.
We have an open issue to rename |
@RichiCoder1 Haven't got around to separate it yet but idd the architect would be removed to keep the adapter lean. I think having a separate adapter per aws service would also keep things lean so in the ALB vs API Gateway scenario we would have
if we would go with The remix guys also have split their server adapters according to third party service for example the cloudflare adapters
Having said that, I wonder if there is any overlap in the request en response structure that an ALB and API GW requires and if AWS will keep this overlap going further 🤔 So there is an advantage when all the integration code is together in a |
@wingleung I don't have hard data to back this up, but I think the events share enough commonality and don't differ wildly enough to justify separating into their own packages. A converged package with event detection should be more than good enough! At least in the case of pages/workers, the deployment and events are materially different enough to justify it (even if the runtime is largely the same) |
|
@RichiCoder1 I kickstarted Remix AWS where I removed the Architect part and focused on adding support for the application load balancer. Almost every property the ALB needs is already being used in the API Gateway v1. I've added some more info about this in the readme. Event detection itself could be an iteration on this idd. |
|
@kamtugeza raises a good point in wingleung/remix-aws#2. the only thing we would want to move to another package would be the adapters. a |
|
Housekeeping! I'm going to close this for now, as it seems like the consensus is that this should be maintained as a community package outside of Remix. Feel free to ping if you feel I missed any context and we need to reconsider. |
Closes: #2696
Testing Strategy:
added jest tests for...
Just a proposal to get this started. I need API Gateway V1 integration because it supports features we need that are not available in v2