-
-
Notifications
You must be signed in to change notification settings - Fork 129
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
fix: edge case when rsa depends on another rsa #785
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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. |
@dai-shi ready to review |
packages/waku/src/lib/hono/runner.ts
Outdated
@@ -28,6 +28,12 @@ export const runner = (options: MiddlewareOptions): MiddlewareHandler => { | |||
.map(async (middleware) => (await middleware).default(options)), | |||
), | |||
); | |||
|
|||
(globalThis as any).__WAKU_HACK_IMPORT__ = async (id: string) => { |
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.
__WAKU_HACK_IMPORT__
is unused, right?
btw, I don't like this hack. there must be a better way.
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.
yes, React will read buildConfig and use WAKU_HACK_IMPORT to load the react server action
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.
Alright, __WAKU_HACK_IMPORT__
should only be required for client. Let me see if I can do something.
This reverts commit c45f2cc.
@@ -189,9 +189,9 @@ export async function renderRsc( | |||
) { | |||
// XXX This doesn't support streaming unlike busboy | |||
const formData = parseFormData(bodyStr, contentType); | |||
args = await decodeReply(formData); | |||
args = await decodeReply(formData, bundlerConfig); |
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 actually wonder why this works. This is basically resolving client entry. Can it be used for server entry?
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 think it's also resolving the server entry, yes
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.
check react test case, they have cases to load server modules
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.
In bundlerConfig
, we use resolveClientEntry
, so even if it works, it's confusing.
@@ -21,7 +21,7 @@ globalThis.__WAKU_${type}_CHUNK_LOAD__ ||= ( | |||
if (!globalThis.__WAKU_${type}_MODULE_LOADING__.has(id)) { | |||
globalThis.__WAKU_${type}_MODULE_LOADING__.set( | |||
id, | |||
customImport(id).then((m) => { | |||
(customImport ? customImport(id) : import(id)).then((m) => { |
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 think this won't work because node condition is not including react-server
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.
that's why you need a vite server to load the module
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.
and import() doesn't support .ts(x) file in dev side
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.
Okay, but it should work on PRD, right?
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 think so, but ID is not correct; it is now like /path/to/file
which is presented as HTTP resource
This reverts commit 65c8c2d.
|
const id = filePathToFileURL(fileId); | ||
if (fileId.startsWith('@id/assets/')) { | ||
const id = '.' + fileId.slice('@id'.length); | ||
return { id, chunks: [id], name, async: true }; |
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.
it will be ./assets/rsf0.js#func
and because rsdw.js is in dist/rsdw.js, it import() will work
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.
This looks pretty good! Thanks for your work!
No description provided.