-
Notifications
You must be signed in to change notification settings - Fork 10
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: migrate remix-netlify adapter #83
feat: migrate remix-netlify adapter #83
Conversation
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Thanks for this @mcansh! I already have an E2E test suite for the Netlify Remix Edge Runtime that I can use for this adapter as well. |
sweet! i can bring over our current tests too if you'd like |
Yeah, the unit tests would be great! Thanks! |
not sure what's up with some types, i'll look more into it tomorrow Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
The E2E tests are good, just trying to figure out why when I approve the run for an external contributor it still borks. |
maybe it was related to this being a "Draft PR" up until now. |
package.json
Outdated
@@ -5,6 +5,7 @@ | |||
"main": "index.js", | |||
"workspaces": [ | |||
"packages/remix-edge-adapter", | |||
"packages/remix-serverless-adapter", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
naming (blocking): It's explicit, but we've discussed internally before about the name of this whenever we'd get around to it, and we landed on @netlify/remix-adapter
@@ -0,0 +1,12 @@ | |||
import { describe, expect, it } from 'vitest' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for bringing in all the tests! 😍
Signed-off-by: Logan McAnsh <logan@mcan.sh>
@nickytonline Is there anything that still needs to be done before we can merge this one? |
I've had a few things on the go. Will look to getting this reviewed. |
@mcansh and @MichaelDeBoey, looking to get this merged next week. I've made a bunch of changes myself, so I can't approve it. I'll let someone on my team do that. Thanks for the work on this both of you! |
Co-authored-by: Michaël De Boey <info@michaeldeboey.be>
Co-authored-by: Michaël De Boey <info@michaeldeboey.be>
There was a problem hiding this 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'm not a Remix expert!
❌ Deploy Preview for hydrogen-demo-site failed.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Note that for the time being, it's expected that the E2E tests fail for a PR from an external contributor. They don't get access to the token to run them. I've checked and they do pass when running manually.
For Serverless Demo
npm run build:demo
ntl serve
to start the serverless demo site.npm run e2e
to start up Cypress.For Edge Demo
npm run build:edge-demo
ntl serve
to start the serverless demo site.npm run e2e
to start up Cypress.For us to review and ship your PR efficiently, please perform the following steps:
ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a
typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
guide and passes our tests.
A picture of a cute animal (not mandatory, but encouraged)