-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Simplify vue appEntrypoint handling #9362
Conversation
🦋 Changeset detectedLatest commit: da8cfef The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Regarding #6827, we'll need to have access to imports to log a warning. Won't this be too hard with those changes? |
I don't think it'll be correct to access the imports that way to log a warning. The imports can go recursively too. I don't quite understand what's happening in #6827, but it seems like our CSS handling isn't working right when it's coming from integration client/server scripts. |
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 seems much simpler, thanks for the PR!
} | ||
if (options?.appEntrypoint) { | ||
const appEntrypoint = options.appEntrypoint.startsWith('.') | ||
? path.resolve(root, options.appEntrypoint) |
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.
root
is a URL
, does path.resolve
automatically convert it to a file path?
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 currently retrieving the root
from Vite's configResolved
hook (which Astro passes), so it should already be a string path.
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.
Comments resolved, approved!
Changes
Simplify
appEntrypoint
handling. Followup from #8794 and #9333This change moves the default export check to runtime in dev. While this means we're unable to use the Astro logger, we can avoid the
this.resolve
,this.load
, andssrLoadModule
calls which can be expensive.With this PR, in dev we will log a warning if
default
is not exported. In build, Rollup will log a warning instead thatdefault
is not exported.Testing
Existing tests should pass.
Docs
n/a. internal change.