-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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: vite runtime renamed to module runner #16137
feat: vite runtime renamed to module runner #16137
Conversation
Run & review this pull request in StackBlitz Codeflow. |
d77022e
to
3c46f97
Compare
/ecosystem-ci run |
📝 Ran ecosystem CI on
✅ histoire, ladle, laravel, marko, qwik, rakkas, sveltekit, unocss, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-pages, vite-plugin-react-swc, vite-setup-catalogue, vitepress |
"./runtime": { | ||
"types": "./dist/node/runtime.d.ts", | ||
"import": "./dist/node/runtime.js" | ||
"./module-runner": { |
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.
module-runner
doesn't feel that right with the two words. I wonder if we should have something more generic here. Is there other things that may be imported by runtimes later on?
Should we keep it as:
import { ModuleRunner } from 'vite/runtime'
with the meaning that it the entry point for Runtimes to import from?
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.
module-runner
was something that you proposed 😄
Is there other things that may be imported by runtimes later on?
No, the idea is to keep this as small as possible. Making it generic would encourage putting more things here.
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 good re-proposing stuff 😅
I don't think it will encourage us to put more things there. I kind of like vite/runtime
better here. But we can move forward and later on rename the things if we want. Maybe @sapphi-red or others can chip in to see what sounds better to them.
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 vote to module-runner
. I'm fine with runner
, too.
runtime
feels a bit different to me. In the new environment API, I understand that the term "runtime" means the actual runtime (e.g. node
, deno
, workerd
) and not the module runner/loader/executor built using Vite's API.
/ecosystem-ci run vite-plugin-vue |
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.
Leaving approval, @sheremet-va feel free to merge whenever you want this. I'm not merging it in case you want to get a review from @sapphi-red first.
Note: the same CI tests are failing in the target branch.
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.
LGTM 👍
Description
This PR renames "Runtime API" into "Runner API".
I also removed
executeEntrypoint
as a method and renamedexecuteUrl
toimport
. Runner API expects "triggeredBy" in "full-reload" event.full-reload
now automatically finds the entrypoint. If you don't want to support HMR, create a runner withhmr: false
.This PR does not add support for granular HMR handling.
Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).