-
-
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
Head propagation #5511
Head propagation #5511
Conversation
🦋 Changeset detectedLatest commit: 2a7f1d7 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 |
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.
Tried to digest this and it looks great aside from some comments below
dc2196c
to
d93a668
Compare
!bench |
!bench |
1 similar comment
!bench |
Node: 14 Node: 16 |
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.
Two small nits, but otherwise LGTM.
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.
Small nit and a clarifying question, but propagreat work!
} | ||
} | ||
|
||
export function renderHead(result: SSRResult) { |
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.
nit: maybe name createRenderHead
to clarify this returns another function?
|
||
const root = new URL('../../fixtures/alias/', import.meta.url); | ||
|
||
describe('head injection', () => { |
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.
Couldn't quite tell from these test cases, but will head injection support inline styles alongside standard stylesheets? I know that renderEntry
needs to handle both inline styles and links from the render result today, since MDX outputs as inline styles in dev, stylesheets in production.
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 makes it possible to create a component that does head injection. So renderEntry
will need to use the right APIs, such as the createComponent
object form, to return the CSS it needs.
So if I'm understanding you right, that work will be done in the renderEntry PR.
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.
@matthewp got it. I'm asking since I know a component may want to inject an inline style
or a link
tag. It sounds like this API gives you the freedom to inject any HTML element + element children that you want?
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.
Right, you can return any string or iterable, essentially the same thing you can do in a template. This PR contains a helper for rendering a stylesheet: https://github.com/withastro/astro/pull/5511/files#diff-a2d99b846ae3297fe01c808e11546d92b906ffafdbc17134bca73edf276fd3b5R17
I'll probably add more helpers in the other PR.
* Head propagation * Adding a changeset * Fix broken build * Self review stuff * Use compiler prerelease exact version * new compiler version * Update packages/astro/src/vite-plugin-head-propagation/index.ts Co-authored-by: Bjorn Lu <[email protected]> * Use getAstroMetadata * add .js * make relative lookup work on win * Use [email protected] * PR review comments * Make renderHead an alias for a better named function Co-authored-by: Bjorn Lu <[email protected]>
Changes
renderEntry
. In the future Astro components could be marked as doing head injection by the compiler.Testing
renderEntry
will do as well.Docs
renderEntry
and in the future, a head propagation API.