Skip to content
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

Merged
merged 6 commits into from
Dec 15, 2023
Merged

Simplify vue appEntrypoint handling #9362

merged 6 commits into from
Dec 15, 2023

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Dec 7, 2023

Changes

Simplify appEntrypoint handling. Followup from #8794 and #9333

This 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, and ssrLoadModule 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 that default is not exported.

Testing

Existing tests should pass.

Docs

n/a. internal change.

Copy link

changeset-bot bot commented Dec 7, 2023

🦋 Changeset detected

Latest 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

@github-actions github-actions bot added pkg: vue Related to Vue (scope) pkg: integration Related to any renderer integration (scope) pr: docs A PR that includes documentation for review labels Dec 7, 2023
@florian-lefebvre
Copy link
Member

Regarding #6827, we'll need to have access to imports to log a warning. Won't this be too hard with those changes?

@bluwy
Copy link
Member Author

bluwy commented Dec 7, 2023

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.

Copy link
Member

@natemoo-re natemoo-re left a 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)
Copy link
Member

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?

Copy link
Member Author

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.

packages/integrations/vue/src/index.ts Show resolved Hide resolved
packages/integrations/vue/src/index.ts Show resolved Hide resolved
Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments resolved, approved!

@ematipico ematipico merged commit a5db8d2 into main Dec 15, 2023
13 checks passed
@ematipico ematipico deleted the vue-app-entrypoint branch December 15, 2023 17:19
@astrobot-houston astrobot-houston mentioned this pull request Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope) pkg: vue Related to Vue (scope) pr: docs A PR that includes documentation for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants