-
Notifications
You must be signed in to change notification settings - Fork 7
refactor(fullstack): use virtual runtime entry #1297
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
base: main
Are you sure you want to change the base?
Conversation
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.
Good point. My intent of #1285 was achieving that, but apparently it wasn't fully done. Thanks
packages/fullstack/src/plugin.ts
Outdated
| // virtual:fullstack/runtime | ||
| function runtimeUtils() { | ||
| return /* js */ ` | ||
| export function mergeAssets(...args) { | ||
| const js = uniqBy(args.flatMap((h) => h.js), (a) => a.href); | ||
| const css = uniqBy(args.flatMap((h) => h.css), (a) => a.href); | ||
| const entry = args.filter((arg) => arg.entry)?.[0]?.entry; | ||
| const raw = { entry, js, css }; | ||
| return { ...raw, merge: (...args$1) => mergeAssets(raw, ...args$1) }; | ||
| } | ||
| function uniqBy(array, key) { | ||
| const seen = new Set(); | ||
| return array.filter((item) => { | ||
| const k = key(item); | ||
| if (seen.has(k)) return false; | ||
| seen.add(k); | ||
| return 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.
Hmm, however copying the code string here is not ideal. Let me think.
| if (id === "\0virtual:fullstack/runtime") { | ||
| return fs.readFileSync( | ||
| path.join(import.meta.dirname, "runtime.js"), | ||
| "utf-8", | ||
| ); | ||
| } |
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.
@pi0 How about this? I think nft style bundling supports something like this, but is nitro itself bundled to support 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.
This will cause pulling in extra dependencies to dist/node_modules + extra dist as nft cannot tree-shake code by imports anymore. I have noticed plugin depends on srvx 0.8 for example it was tracing both 0.8 and 0.9!
Maybe later we can have a build step to convert runtime.ts into inlined code?
Context: In nitro, we bundle most of plugin dependencies. This causes an issue with using fullastack plugin in end projects:
This PR fixes the limitation by exposing small set of runtime utils as a virtual entry instead.
I have preserved other
/runtimesubpath for external usage without breaking change.