feat(create-pages): api handling #1108
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
|
||
| export type CreateApi = <Path extends string>(params: { | ||
| path: Path; | ||
| render: 'static' | 'dynamic'; |
There was a problem hiding this comment.
I'm not sure if we call render for api.
There was a problem hiding this comment.
changed to mode for now... let me know what you think
There was a problem hiding this comment.
I'm neutral. @sandren may have an idea.
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
| path, | ||
| typeof window !== 'undefined' | ||
| ? window.location.href | ||
| : 'http://localhost', // FIXME what should this be? |
There was a problem hiding this comment.
@dai-shi do you know how this should be handled? Request needs a valid url, which seems to require a host. When we run the tests localhost is the "host".
There was a problem hiding this comment.
I think having always localhost is okay. When we learn more use cases, we can reconsider. As a workaround, we could use unstable_getHeaders for "host".
new URL(path, 'http://localhost')There was a problem hiding this comment.
hm I get this when calling unstable_getHeaders from createPages.handleApi
I'm ok with leaving it as localhost though.
There was a problem hiding this comment.
Hmm, I'm not sure why it errors. let's look into it separately.
Actually, you can probably read "host" header from params.headers, but if it's not available, we can fallback to localhost.
There was a problem hiding this comment.
like this?
const req = new Request(
new URL(path, options.headers['host'] ?? 'http://localhost'),
options,
);There was a problem hiding this comment.
I'm not sure but should we need the protocol part too?
There was a problem hiding this comment.
I think it's needed? This is the output when run through my browser console
There was a problem hiding this comment.
Yes, I mean the same and options.headers['host'] doesn't include the protocol part, right?
There was a problem hiding this comment.
Good point about the protocol.
However, handApi will run on our server, right?
So will we always want to request to http://localhost actually? I think it makes sense to remove the use of the host header since we expect this to request from the server this code executes on.
Correct me if I'm off here though.
There was a problem hiding this comment.
It runs on our server (dynamic) or the build time (static).
I could imagine some use cases would check "host" and change the behavior based on it. So, at least they should be able to read the "host" header.
At that point, creating URL with the "host" header makes sense, but we can defer the decision until we see clear requirements.
tylersayshi
force-pushed
the
create-pages-api
branch
from
1621a2f to
f2660af
Compare
| @@ -147,6 +147,15 @@ export type CreateLayout = <Path extends string>( | |||
| }, | |||
| ) => void; | |||
|
|
|||
| type Method = 'GET' | 'POST' | 'PUT' | 'DELETE'; | |||
There was a problem hiding this comment.
I don't mind, but any reason for this choice?
There was a problem hiding this comment.
I was hoping just to make this typesafe, but happy to add more methods or remove it if it gets in the way
This adds api route handling in createPages with tests