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

Fix hoisted script propagation in content collection pages #6119

Merged
merged 9 commits into from
Feb 6, 2023
Merged

Conversation

matthewp
Copy link
Contributor

@matthewp matthewp commented Feb 3, 2023

Changes

Testing

  • Unskipped previously failing test

Docs

N/A

@changeset-bot
Copy link

changeset-bot bot commented Feb 3, 2023

🦋 Changeset detected

Latest commit: 9df9d3a

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 the pkg: astro Related to the core `astro` package (scope) label Feb 3, 2023
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.

Can't say I grok this fully, but the test looks good and I'm cool with the approach. LGTM!

Copy link
Contributor

@bholmesdev bholmesdev left a comment

Choose a reason for hiding this comment

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

Couple comments, mostly for cleaning up code I wrote originally. Propagreat stuff!

@@ -22,6 +22,7 @@ export interface PageBuildData {
moduleSpecifier: string;
css: Map<string, { depth: number; order: number }>;
contentCollectionCss: Map<string, Set<string>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Meant to rename contentCollectionCss to propagatedCss or propagatedStyles for consistency. Possible to add to this PR?

Comment on lines 43 to 46
ctx: { getModuleInfo: GetModuleInfo },
walkUntil?: (importer: string) => boolean
): Generator<[ModuleInfo, number, number], void, unknown> {
for (const res of walkParentInfos(id, ctx)) {
for (const res of walkParentInfos(id, ctx, walkUntil)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This refactor is unused looks like. Good to revert changes to this file!

);
}
if (entryScripts.size) {
const entryFileNames = new Set<string>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Love that we can look these up now 👏 Way simpler than scary naming conventions!

@matthewp matthewp merged commit 2189170 into main Feb 6, 2023
@matthewp matthewp deleted the cc-hoist2 branch February 6, 2023 15:42
@astrobot-houston astrobot-houston mentioned this pull request Feb 6, 2023
This pull request was closed.
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants