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!: upgrade to Remix 2 #191

Merged
merged 21 commits into from
Oct 19, 2023
Merged

feat!: upgrade to Remix 2 #191

merged 21 commits into from
Oct 19, 2023

Conversation

ascorbic
Copy link
Contributor

@ascorbic ascorbic commented Oct 16, 2023

Description

Update adapters and edge runtime to Remix 2, and to Functions API v2. Switches to using our custom server during dev in edge, now that there's better support for this. Moves everything to ESM now that's required.

There is a separate PR to update the template once this is out.

Related Tickets & Documents

  • Related Issue #
  • Fixes FRA-48

QA Instructions, Screenshots, Recordings

Please replace this line with instructions on how to test your changes

For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code 🧑‍💻. This
    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.
  • Read the contribution guidelines 📖. This ensures your code follows our style
    guide and passes our tests.
  • Update or add tests (if any source code was changed or added) 🧪
  • Update or add documentation (if features were changed or added) 📝
  • Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

@ascorbic ascorbic requested a review from a team as a code owner October 16, 2023 20:12
@netlify
Copy link

netlify bot commented Oct 16, 2023

Deploy Preview for remix-serverless ready!

Name Link
🔨 Latest commit 942bf87
🔍 Latest deploy log https://app.netlify.com/sites/remix-serverless/deploys/65315c60caafab00086dbc06
😎 Deploy Preview https://deploy-preview-191--remix-serverless.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify

This comment was marked as outdated.

@netlify
Copy link

netlify bot commented Oct 16, 2023

Deploy Preview for remix-edge ready!

Name Link
🔨 Latest commit 942bf87
🔍 Latest deploy log https://app.netlify.com/sites/remix-edge/deploys/65315c6085542f0008e65f40
😎 Deploy Preview https://deploy-preview-191--remix-edge.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions github-actions bot added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Oct 16, 2023
@ascorbic ascorbic marked this pull request as draft October 16, 2023 21:03
@ascorbic ascorbic force-pushed the v2 branch 5 times, most recently from 80e3fa6 to 5a9314e Compare October 17, 2023 15:34
Comment on lines -35 to -36
- name: Run unit tests
run: npm test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were just the Netlify adapter tests ported from Remix repo. See below for why they were removed.

Comment on lines +1 to +3
{
"extends": ["@remix-run/eslint-config", "@remix-run/eslint-config/node"],
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to json because of ESM

Copy link
Contributor

Choose a reason for hiding this comment

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

ESLint v9 is going to remove support for JSON .eslintrc files

@@ -1,10 +1,17 @@
import * as build from '@remix-run/dev/server-build'
import { createRequestHandler } from '@netlify/remix-adapter'
import { installGlobals } from '@remix-run/node'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't want to use Remix versions of web streams

request: Request,
responseStatusCode: number,
responseHeaders: Headers,
remixContext: EntryContext,
loadContext: AppLoadContext,
) {
const markup = renderToString(<RemixServer context={remixContext} url={request.url} />)
const body = await renderToReadableStream(<RemixServer context={remixContext} url={request.url} />, {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Streaming works now

Comment on lines +50 to +51
// Avoid a bug where responses aren't flushed if there's an outstanding timer.
clearTimeout(timer)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ignoredRouteFiles: ['**/.*'],
server: process.env.NETLIFY || process.env.NETLIFY_LOCAL ? './server.ts' : undefined,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can now use our custom server in dev

@@ -1,25 +1,30 @@
/**
* By default, Remix will handle generating the HTTP Response for you.
* You are free to delete this file if you'd like to, but if you ever want it revealed again, you can run `npx remix reveal` ✨
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not true!

"scripts": {
"prebuild": "cd ../../ && npm run build -w=packages/remix-runtime && npm run build -w=packages/remix-edge-adapter && npm run build -w=packages/remix-adapter",
"build": "remix build",
"dev": "remix watch",
"dev": "remix dev --manual -c \"ntl dev --framework=#static\"",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The remix server now runs the netlify CLI rather than the other way around

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests were all for the workarounds that translated the Remix responses into Netlify/Lambda-compatible ones. These are no longer needed with Functions v2, because we can return the Reponse directly

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 now effectively the same as the edge server

@ascorbic ascorbic marked this pull request as ready for review October 18, 2023 07:48
@ascorbic
Copy link
Contributor Author

ascorbic commented Oct 18, 2023

❌ Deploy Preview for hydrogen-demo-site failed.

Name Link
🔨 Latest commit 77694d9
🔍 Latest deploy log https://app.netlify.com/sites/hydrogen-demo-site/deploys/652f7277b6c7780008aff8cd

This site no longer exists. I've disabled builds.

pieh
pieh previously approved these changes Oct 19, 2023
Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

Seems good - need to resolve package-lock.json conflict and will have to re-approve after that

pieh
pieh previously approved these changes Oct 19, 2023
@ascorbic ascorbic merged commit 81b169f into main Oct 19, 2023
14 checks passed
@ascorbic ascorbic deleted the v2 branch October 19, 2023 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants