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

Support re-exporting astro components containing client components #3625

Merged
merged 20 commits into from
Jun 21, 2022

Conversation

matthewp
Copy link
Contributor

@matthewp matthewp commented Jun 17, 2022

Changes

Testing

  • Tests added

Docs

N/A, perf improvement

@changeset-bot
Copy link

changeset-bot bot commented Jun 17, 2022

🦋 Changeset detected

Latest commit: 1476acc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
astro Patch
@astrojs/lit Minor
@e2e/lit-component Patch

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 the pkg: astro Related to the core `astro` package (scope) label Jun 17, 2022
@github-actions github-actions bot added pkg: lit Related to Lit (scope) pkg: integration Related to any renderer integration (scope) labels Jun 17, 2022
@matthewp matthewp marked this pull request as ready for review June 20, 2022 13:41
@@ -71,30 +71,18 @@ export async function collectPagesData(
css: new Set(),
hoistedScript: undefined,
scripts: new Set(),
preload: await ssrPreload({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This removes the "preload" step which was the expensive part of the build that we no longer need to do. Metadata is extracted via the compiler instead.

...internals.discoveredClientOnlyComponents,
...astroConfig._ctx.renderers.map(r => r.clientEntrypoint).filter(a => a) as string[],
...internals.discoveredScripts,
]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are all of the client pieces to build; hydrated and client:only components, renderers, and hoisted scripts.

@@ -138,6 +89,9 @@ export async function staticBuild(opts: StaticBuildOptions) {
await cleanSsrOutput(opts);
}
} else {
// Inject the manifest
await injectManifest(opts, internals)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The SSR manifest is injected after both builds, so that we can build a full manifest containing all of the asset information.

return JSON.stringify(manifest);
});
internals.ssrEntryChunk = chunk;
delete bundle[chunkName];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The SSR entry is removed from the bundle so that we can inject the manifest post client build.

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.

Very happy to see this! Do you have any benchmarks to measure the perf impact?

Non-blocking, but a bit worried about the custom elements change... Might have some ideas that mostly avoid a breaking change, happy to discuss.

.changeset/unlucky-hairs-camp.md Outdated Show resolved Hide resolved
Comment on lines +109 to +119
const injectPlugin: VitePlugin = {
name: '@astrojs/netlify/plugin-inject',
generateBundle(_options, bundle) {
if(_buildConfig.serverEntry in bundle) {
const chunk = bundle[_buildConfig.serverEntry];
if(chunk && chunk.type === 'chunk') {
chunk.code = `globalThis.process = { argv: [], env: {}, };${chunk.code}`;
}
}
}
};
Copy link
Member

Choose a reason for hiding this comment

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

Is this related?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the Netlify Edge functions were failing in CI, but I think this was broken at the time I created this branch. Looks like the tests were disabled here: #3631

@FredKSchott
Copy link
Member

Re: benchmarks -- definitely run this on https://github.com/rebelchris/ddt-v2 and share the stats/results (if they're on a recent enough beta).

@matthewp
Copy link
Contributor Author

matthewp commented Jun 20, 2022

Before

02:31:26 PM [build] 928 page(s) built in 170.82s
02:31:26 PM [build] Complete!

After

02:47:14 PM [build] 928 page(s) built in 114.65s
02:47:14 PM [build] Complete!

32% faster. Note that this PR does not prevent getStaticPaths loading in vite ssr mode. Doing that would likely improve it even more. I wonder if we could do that in the generate step instead of upfront. Something for a future PR.

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.

Just noticed the drafts change, are we still handling that somewhere?

Comment on lines -117 to -127
// Filter pages by using conditions based on their frontmatter.
Object.entries(allPages).forEach(([page, data]) => {
if ('frontmatter' in data.preload[1]) {
// TODO: add better type inference to data.preload[1]
const frontmatter = (data.preload[1] as any).frontmatter;
if (Boolean(frontmatter.draft) && !this.config.markdown.drafts) {
debug('build', timerMessage(`Skipping draft page ${page}`, this.timer.loadStart));
delete allPages[page];
}
}
});
Copy link
Member

Choose a reason for hiding this comment

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

Does this break drafts support? Didn't see where this was handled elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I was thinking this was handled in the markdown plugin but I don't see the code for it. There are tests which are passing though. Will investigate more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test was just wrong, so this is broken in this branch. Will fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 2dc17d0

.changeset/unlucky-hairs-camp.md Outdated 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.

Approved! Nice work.

Feel like this perf boost deserves a callout with an astro changeset!

@matthewp matthewp merged commit f5afaf2 into main Jun 21, 2022
@matthewp matthewp deleted the re-export-component-client branch June 21, 2022 12:32
@github-actions github-actions bot mentioned this pull request Jun 21, 2022
SiriousHunter pushed a commit to SiriousHunter/astro that referenced this pull request Feb 3, 2023
…ithastro#3625)

* Support re-exporting astro components containing client components

* Include metadata for markdown too

* Fix ssr, probably

* Inject post-build

* Remove tagName custom element test

* Allows using the constructor for lit elements

* Fix hoisted script scanning

* Pass through plugin context

* Get edge functions working in the edge tests

* Fix types for the edge function integration

* Upgrade the compiler

* Upgrade compiler version

* Better release notes for lit

* Update .changeset/unlucky-hairs-camp.md

Co-authored-by: Nate Moore <[email protected]>

* Properly test that the draft was not rendered

* Prevent from rendering draft posts

* Add a changeset about the build perf improvement.

Co-authored-by: Nate Moore <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pkg: integration Related to any renderer integration (scope) pkg: lit Related to Lit (scope)
Projects
None yet
3 participants