-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
chore: replace netlify-cli with @netlify/dev
#14686
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
Merged
Merged
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
5c34988
chore: replace netlify-cli with netlify dev package
benmccann 6d72af8
Merge branch 'main' into netlify-dev
teemingc 19c7e93
update lockfile
teemingc 95b9b4b
match modern netlify build output
teemingc 7db3d52
add doc
teemingc 767093c
remove srvx because it doesn't work with node 18
teemingc 75719ac
format
teemingc 028349b
switch to node
teemingc 776ee13
fix format and lint ignore
teemingc a49bfbc
no more srvx
teemingc 65c6d01
try installing deno for netlify edge in github action
teemingc b9dff12
oops indentation
teemingc 8ca4769
pass port in through npm script
teemingc 8cdd066
remove port from npm script
teemingc 8944bce
log when server started
teemingc 24c1cee
Merge `origin/main` into `netlify-dev`
teemingc b579c6c
prettier
teemingc 72021fb
generate types
teemingc File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,2 @@ | ||
| [dev] | ||
| publish = "build" | ||
| publish = "build" |
39 changes: 39 additions & 0 deletions
39
packages/adapter-netlify/test/apps/basic/netlify/functions/render.mjs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| // This is a temporary workaround to be compatible with Netlify's new dev server | ||
| // TODO: remove this once we overhaul the Netlify adapter to use Netlify's new serverless function format https://docs.netlify.com/build/functions/get-started/?data-tab=TypeScript#write-a-function | ||
|
|
||
| import { handler } from '../../.netlify/functions-internal/sveltekit-render.mjs'; | ||
|
|
||
| /** | ||
| * @param {Request} request | ||
| * @param {import('@netlify/functions').HandlerContext} context | ||
| */ | ||
| export default async function (request, context) { | ||
| const [rawUrl, rawQuery] = request.url.split('?'); | ||
| /** @type {import('@netlify/functions').HandlerEvent} */ | ||
| const event = { | ||
| rawUrl, | ||
| rawQuery: rawQuery || '', | ||
| headers: Object.fromEntries(request.headers), | ||
| httpMethod: request.method, | ||
| isBase64Encoded: false, | ||
| path: new URL(request.url).pathname, | ||
| queryStringParameters: Object.fromEntries(new URL(request.url).searchParams), | ||
| body: request.body && (await request.text()), | ||
| multiValueHeaders: {}, | ||
| multiValueQueryStringParameters: null | ||
| }; | ||
| const result = await handler(event, context); | ||
| if (result) { | ||
| return new Response(result.body, { | ||
| status: result.statusCode, | ||
| // @ts-ignore | ||
| headers: result.headers | ||
| }); | ||
| } | ||
|
|
||
| return new Response('Not Found', { status: 404 }); | ||
| } | ||
|
|
||
| export const config = { | ||
| path: '/*' | ||
| }; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,15 @@ | ||
| [dev] | ||
| publish = "build" | ||
| publish = "build" | ||
|
|
||
| # TODO: remove these once we overhaul the Netlify adapter to use the new edge declarations https://docs.netlify.com/build/edge-functions/declarations/#declare-edge-functions-inline | ||
|
|
||
| [build] | ||
| # defaults to "netlify/edge-functions" (without the . prefix) | ||
| edge_functions = ".netlify/edge-functions" | ||
|
|
||
| # the dev server doesn't read the manifest.json in edge-functions so we need | ||
| # to explicitly declare this here | ||
| [[edge_functions]] | ||
| path = "/*" | ||
| function = "render" | ||
| excludedPath = ["/_app/immutable/*", "/_app/version.json", "/.netlify/*"] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| // This allows us to run `netlify dev` without installing the Netlify CLI | ||
| // Don't include this file in type checking because @netlify/dev has transitive imports with type errors | ||
| import { NetlifyDev } from '@netlify/dev'; | ||
| import http from 'node:http'; | ||
| import process from 'node:process'; | ||
| import { getRequest, setResponse } from '@sveltejs/kit/node'; | ||
|
|
||
| const netlifyDev = new NetlifyDev({}); | ||
|
|
||
| const serverReady = netlifyDev.start(); | ||
|
|
||
| const port = process.env.PORT ? +process.env.PORT : 8888; | ||
| const base = `http://localhost:${port}`; | ||
|
|
||
| http | ||
| .createServer(async (req, res) => { | ||
| await serverReady; | ||
| const request = await getRequest({ request: req, base }); | ||
| const response = | ||
| (await netlifyDev.handle(request)) ?? new Response('Not Found', { status: 404 }); | ||
| await setResponse(res, response); | ||
| }) | ||
| .listen(port); | ||
| console.log(`Netlify Dev listening on http://localhost:${port}`); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I'm not very familiar with GitHub actions and CI. Installing deno this way solves our issue explained here #14686 (comment) . Does adding this have any implications I'm not aware of?
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.
not a fan of this. why can't it run on node? If we allow this, would we consider adding others for vercel, bun or whatnot in the future?
if we want to test the runtime part of adapters, it should probably be in a separate workflow to create tailored environments for the adapter. The one here is not really matching netlify at all as there's not only deno but a host of other things that isn't present there.
Uh oh!
There was an error while loading. Please reload this page.
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.
Netlify edge functions run on the Deno runtime so it tries to install it when spinning up a Netlify edge function for the preview server. Currently, we have workerd too for Cloudflare that's installed along with Wrangler when we run Cloudflare tests. I don't know why
@netlify/devdoesn't work similarly.So far, Vercel has nothing we can run locally so I think we're suppose to set up deployments for tests at some point (or maybe we should do this for all of them?)
Should we split up the adapter tests into their own workflows?
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.
Wouldn't this also require deno for local testing? In this case it would have to be documented in our contributing and i think that makes an even stronger case for separating it out into a separate test script.
Uh oh!
There was an error while loading. Please reload this page.
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.
Surprisingly not. It just works when running it locally but not in CI due to permission settings. Maybe @serhalp can weigh in on how to best handle this. I'll dig into the
@netlify/devcode later to see how it's handledThere 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.
isolation is also about execution time and resources. why add deno to a test matrix with a node dimension if it's hardly used?
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.
I imagine the installation should get cached. Splitting into new jobs also means that we're paying some setup costs in each of the jobs which will burn more total CI minutes
Uh oh!
There was an error while loading. Please reload this page.
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.
I do wonder about Ben's point but I'm not sure where the greater cost lies. It does make a lot of sense to separate the build tests from the runtime tests (especially for cloudflare and netlify edge which will be the exact same in each node matrix). But I think we don't have many runtime tests yet to truly benefit from this.
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.
Ah, avoiding running them on multiple versions of Node when we don't even run them in Node is a pretty good argument. I hadn't thought about that. It will save us total CI minutes and probably finish faster on average, so I'd be in favor if it's something you want to take on
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.
I've created an issue for that here #15175 it will probably be useful when we have more Cloudflare/Netlify tests after we've added the environment API