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

Partials RFC #721

Merged
merged 3 commits into from
Oct 26, 2023
Merged

Partials RFC #721

merged 3 commits into from
Oct 26, 2023

Conversation

matthewp
Copy link
Contributor

@matthewp matthewp commented Oct 3, 2023

Summary

Add the ability to configure a page in the pages directory to be treated as a fragment. A fragment is a chunk of HTML that is not interpreted to be a page, so no head content or doctype are included.

Links

@matthewp matthewp mentioned this pull request Oct 3, 2023
@ZebraFlesh
Copy link

No client-side scripts from Astro will be part of this change.

Can you elaborate on the meaning of this line from the Non-Goals section? My use case is that I want to include client side scripts defined by my fragments, and I'm unclear on if this will/won't be supported.

@philipschm1tt
Copy link

I am interested in fragments coming from the micro-frontend perspective: include a fragment either server-side/edge-side via SSI/ESI or include a fragment client-side via Ajax. I started a roadmap discussion about these use cases at #713
Note: I don't expect these use cases to be covered with this planned partials feature but I think it's desirable to support these use cases in the future and thinking about that future can help inform the current first step towards partials.

The proposal solves some use cases: notably the HTMX/Hotwire-style approach where the server updates the markup for parts of the page.

I want to think one step further about potential future micro-frontend Ajax support.
With Ajax, the "current" page might be rendered by any technologies geared for server-side rendering. Maybe a classic Java + Spring MVC page that uses a Thymeleaf template. Then you might have a button on that page to, for example, get your notification or look into your cart. That could be done via Ajax: the client-side JavaScript could request that fragment from a separate Astro server. The Astro server would then need to deliver not just the markup but also the styles and some JavaScript for that fragment. I believe those stylesheets and scripts are commonly then not added to the head of the page but should work by including the tags "inline" where the fragment is added.

If Astro were to support – in the future – that use case of delivering a fragment to a page rendered by another server, it would need to support styles and scripts for the fragment.
Would you then add another flag like export const fragmentWithScriptsAndStyles that is false by default? Or would you include a second flag like export const fragmentMarkupOnly that is true by default? Or would you use two independent flags like export const fragmentMarkupOnly and export const fragmentWithStyleAndScript?
If you would use two independent flags, the current proposal of just export const fragment could become ambiguous and need some work.

@matthewp matthewp mentioned this pull request Oct 5, 2023
@matthewp
Copy link
Contributor Author

matthewp commented Oct 5, 2023

@ZebraFlesh No, all I mean by that is that Astro is not opinionated about how you load a fragment in the client and are not getting involved in that. So Astro is not going to be adding scripts of its own. You can of course use your own scripts to manage fragment insertion.

@matthewp
Copy link
Contributor Author

matthewp commented Oct 5, 2023

@philipschm1tt Scripts and styles are included without any config value, by default, for all pages. So if you want those things then you don't need this feature. Am I misunderstanding something here?

@philipschm1tt
Copy link

@philipschm1tt Scripts and styles are included without any config value, by default, for all pages. So if you want those things then you don't need this feature. Am I misunderstanding something here?

If I understand the proposal correctly, with a fragment, you would not get any stylesheet or script links as part of the fragment, but you would rely on the page that includes the fragment (where Astro presumably already provided styles and scripts for that page).

What I'm saying is that there is a fragment use case, where the page that includes the fragment does not come from Astro but from a different server. For that use case, it is common that the fragment that you include also contains a stylesheet and script.

For that micro-frontend Ajax approach, I imagine the ideal behavior like this:
The fragment would be rendered like a page but instead of doctype, head, body, ... you would get a partial of the page body. That partial would contain a stylesheet link and script link as part of the fragment that would not be merged into the head of the page.

Here is an excerpt from the book Micro-frontends in Action (with a manning account, you can "unscramble" it for a few minutes): https://livebook.manning.com/book/micro-frontends-in-action/chapter-3/

Their fragment (in this example without JS) looks like this – it includes a stylesheet link in the fragment:

<link href="http://localhost:3002/static/fragment.css" rel="stylesheet" />
<h2>Recommendations</h2>
<div class="recommendations">
  ...
</div>

Just to reiterate: I'm not arguing for perfectly solving micro-frontend scenarios as part of this RFC but to consider in this RFC what the impact might be if you addressed the micro-frontend use cases in the future.

@matthewp
Copy link
Contributor Author

matthewp commented Oct 6, 2023

@philipschm1tt I understand, what i'm saying is that the thing you are describing is not a fragment (in the definition of this RFC). It's just a regular page. You might think of it as a fragment, from the perspective of how you're using it within another framework, but Astro's perspective a page which includes styles is not a fragment.

Does that make sense? What I'm saying is, you should be able to create a page with just the content in your example and it will include styles, as you have shown. Without needing this feature.

@philipschm1tt
Copy link

@philipschm1tt I understand, what i'm saying is that the thing you are describing is not a fragment (in the definition of this RFC). It's just a regular page. You might think of it as a fragment, from the perspective of how you're using it within another framework, but Astro's perspective a page which includes styles is not a fragment.

Does that make sense? What I'm saying is, you should be able to create a page with just the content in your example and it will include styles, as you have shown. Without needing this feature.

Yes, I understand what use case(s) you want to solve with this proposal.
But multiple people in the original discussion #266 had the micro-frontend use case. What bothered them (and me), was the parts of the page that only work at the page level and not as part of a fragment.

These server-side integration use cases (like the Ajax client-side integration approach), still need styles and behavior as part of these kinds of fragment. The fragments in these contexts come from different servers.

@matthewp
Copy link
Contributor Author

matthewp commented Oct 6, 2023

@philipschm1tt Can you explain why the existing pages folder behavior does not work for your use-case?

@philipschm1tt
Copy link

@philipschm1tt Can you explain why the existing pages folder behavior does not work for your use-case?

I have not personally explored this as much as the commenters in #266 and some of my colleagues.

What I'm looking for is a response like

<link href="fragment.css" rel="stylesheet" />
<script defer src="fragment.js"></script>
<h2>Recommendations</h2>
<div class="recommendations">
  ...
</div>

So roughly the same behavior as for a page but without doctype and with the CSS/JS links for that "page" outside of a head tag.

And unlike some of the commenters on that issue, I would like to use SSR and could not use some of the post-processing workarounds that were suggested for static files.

@matthewp
Copy link
Contributor Author

matthewp commented Oct 6, 2023

@philipschm1tt If you don't include a <head> tag then there will be no head tag, but there will be the scripts and styles at the top, just as you have written.

The only part that is different is that there will also be a doctype, which you say that you do not want.

I would then ask, how are you dealing with these scripts and styles? Are you not extracting them out and putting them into the head? If you do not, you will get FOUC.

@ZebraFlesh
Copy link

ZebraFlesh commented Oct 6, 2023

I am inlining those styles for the microfrontend use case. Today, I do the following:

  1. manually inline CSS
  2. remove DOCTYPE
  3. remove head tag (but retain contents)
  4. remove body tag (but retain contents)

And now I have a fragment. (I could skip the inlining, but since I'm doing ESI processing at edge on a streaming response, I can't "go back" and put the CSS in the head tag, because that content has already been streamed to the client.)

My hope for this feature was removing the last 3 steps because Astro would be smart enough to not add them.

@matthewp
Copy link
Contributor Author

matthewp commented Oct 6, 2023

Astro does not add a head or body tag. So those steps you already can skip by simply not adding those tags yourself.

By (1) do you mean you take the link tags from the HTML and move them into the head?

@matthewp
Copy link
Contributor Author

matthewp commented Oct 6, 2023

This feature can be tried out by installing the preview release:

npm install astro@next--fragments

@philipschm1tt
Copy link

philipschm1tt commented Oct 6, 2023

The only part that is different is that there will also be a doctype, which you say that you do not want.

Yes. I think that's why that was the focus of #266
I think the SSI/ESI user might be helped best with a simple "please don't include doctype" config.

I would then ask, how are you dealing with these scripts and styles? Are you not extracting them out and putting them into the head? If you do not, you will get FOUC.

For the Ajax case the fragment is requested client-side on user interaction, so you won't have the styles and scripts during page load.

For the server-side integration case, this may depend on the specific approach you use.
I'm not aware of a "built-in" way with SSI or ESI to move such links to the head. I don't know if some people might include a "fragment" that includes only these links to the head (at the cost of an additional request). I guess with SSI/ESI you may typically either accept some FOUC or use some workarounds to hide content until CSS is loaded (and then trigger the reveal with JS).

I think this is probably a classic SSI/ESI challenge with some options and trade-offs.
It may not be a big deal since the main part of the page should usually come from the server that adds its styles/scripts to the head. And it may benefit from caching the CSS from the second request on.

@ZebraFlesh
Copy link

ZebraFlesh commented Oct 6, 2023

Astro does not add a head or body tag. So those steps you already can skip by simply not adding those tags yourself.

Excellent point. It's been a while since I've looked at this problem, so I was quoting from out post-processing script, but this explanation makes sense.

By (1) do you mean you take the link tags from the HTML and move them into the head?

No, I mean we find each <link rel="stylesheet">, read the indicated file contents, and replace the <link rel="stylesheet"> with <style>{contents_of_file}</style>. This is all done as part of an astro:build:done hook.

@MrHBS
Copy link

MrHBS commented Oct 12, 2023

Consider renaming this feature to partials

@matthewp
Copy link
Contributor Author

@MrHBS definitely open to doing that. Should probably run a pole some where.

@matthewp matthewp changed the title Fragments RFC Partials RFC Oct 17, 2023
@matthewp
Copy link
Contributor Author

I've renamed it to Partials in this RFC. I'm working to also update the code. Additionally I've added a section to the RFC describing why the proposal doesn't include scripts/styles: https://github.com/withastro/roadmap/blob/fragments/proposals/partials.md#including-head-content-but-not-doctype

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

It would be great if we could explain the result with an example. While it's explained that other tools have used partials, some people wouldn't know how they should be used. I am one of them.

I would like a few lines that say if a page opt-ins partial = true, what's the result and what should the user expect from that page?

@matthewp
Copy link
Contributor Author

@ematipico Added that in 47ab4ba, let me know if that's what you were looking for!

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Thank you, @matthewp, for addressing my comment. Happy with the RFC!

@matthewp
Copy link
Contributor Author

I'm moving for a call for consensus on this RFC. This will be the final comment period (3 days); if there are no objections this will be merged and the feature can be released in a future release (likely 3.4)

@silent1mezzo
Copy link

I think my biggest concern is with the variable vs file convention. Having it as a variable could clash with existing pages, third party components/packages and it also makes it more difficult to know whether a certain page is used as a partial or not since you have to investigate all of the pages individually.

Hugo handles partials by requiring them to be within a specific directory which is a little more elegant (albeit more work as mentioned in the RFC).

Any file nested under pages/partials would be treated as a partial and could be removed from routing. IMO this makes it easier to see which files are partials and can't conflict with any variables used in the component script.

@ZebraFlesh
Copy link

Just to provide some flavor, my plan is to use this implementation to build only partials (no actual pages). Think of it as: a build for a set of ESI replacements. Provided that's still possible with a directory-based approach, that looks good from the consumer side.

@matthewp
Copy link
Contributor Author

@silent1mezzo I will probably use a src/pages/partials convention in my own apps, but I don't think Astro should enforce that. For a few reasons:

  • Astro is mostly unopinionated about where you put your files. Instead we lean on conventions; for ex. it's common for layouts to be in src/layouts, but they don't have to be! Layouts are really just normal components. So it's not normal for us to have a folder convention for organizational reasons. If there was a technical reason for it, we would do it.
  • Adding a src/pages/partials known folder would be a breaking change, as people might currently have pages in such a folder.
  • If you didn't know of this feature, and added a page to src/pages/partials it would be confusing to see that the page did not include styles+scripts. Since this is a niche feature, I don't think having people use it by accident is a good thing to do.
  • With the current design, integrations can add partials; they would not be able to do so with a folder or file naming requirement.

@silent1mezzo
Copy link

  • Astro is mostly unopinionated about where you put your files. Instead we lean on conventions; for ex. it's common for layouts to be in src/layouts, but they don't have to be! Layouts are really just normal components. So it's not normal for us to have a folder convention for organizational reasons. If there was a technical reason for it, we would do it.

That's fair

  • Adding a src/pages/partials known folder would be a breaking change, as people might currently have pages in such a folder.

Won't adding the partials variable also be breaking since someone could have that already?

  • If you didn't know of this feature, and added a page to src/pages/partials it would be confusing to see that the page did not include styles+scripts. Since this is a niche feature, I don't think having people use it by accident is a good thing to do.

Hmm, I actually disagree with this. I think the variable is more confusing than a folder. At best they're the same and you need to go to the docs to figure it out. At least the folder has examples from other frameworks people could lean on.

  • With the current design, integrations can add partials; they would not be able to do so with a folder or file naming requirement.

Totally fair

@matthewp
Copy link
Contributor Author

Won't adding the partials variable also be breaking since someone could have that already?

No, exporting variables is not supported by Astro component outside of the documented ones (getStaticPaths, prerender), so we can add new variables as needed.

Hmm, I actually disagree with this. I think the variable is more confusing than a folder. At best they're the same and you need to go to the docs to figure it out. At least the folder has examples from other frameworks people could lean on.

A better explanation for my point here is that in Astro you can create any URL structure you want, and if we give special meaning to pages/partials that means that you cannot create a page with the URL /partials/whatever.

Conversely we would be enforcing a URL structure for partials themselves. Some people might want to do RESTful URLs where /todos is a partial response with the list of todos. I don't think it's Astro's place to tell people what their URLs can be.

@silent1mezzo
Copy link

Makes sense

@lloydjatkinson
Copy link

lloydjatkinson commented Oct 24, 2023

Just wanted to comment I find the name of the feature unclear (though I understand how the name was arrived at). Partials is already a fairly well used terminology to describe reusable templates or "views" particularly in MVC-like frameworks. These partials have little in common with this RFC. Essentially partials in this context are components. As a new user, I would be initially confused.

Partial views in ASP.NET Core
NodeJs Express EJS Layouts and Partials
How to use Partials in Rails

As the unit of abstraction in Astro is the component (not partials), and as I understand it the motivation for this RFC is for endpoints, perhaps Fragment Pages or Page Fragments is a better feature name?

@PeterDraex
Copy link

PeterDraex commented Oct 25, 2023

The section Including head content but not doctype lists a cons of injecting scripts/styles directly mid-page and concludes that it's never the right approach to include scripts/styles in the partial.

In my case, I am working with an old PHP site with spaghetti code, which I'd like to gradually transition to Astro. During the transition, my plan is to have the Astro partials injected in the HTML. Developing code to manage both the PHP assets and Astro assets together is somewhere between not-worth-it and impossible. On the other hand, I can make sure I develop the Astro components in a manner that repeated injection of styles/scripts would not be an issue. If this RFC goes ahead as-is, I'll have to keep setting partial = false and stripping doctype. Unless you guys see any better way to do the transition than server-side injection of Astro code?

As there are many use cases for partials, it might be good to have the option to configure separately:

  • if doctype should be stripped
  • if scripts/styles should be stripped

What do you think?

@matthewp
Copy link
Contributor Author

@PeterDraex I'd recommend stripping out the doctype using middleware for your case. It should be very simple to write. We don't want a feature that immediate breaks down once you use a partial more than once.

@matthewp
Copy link
Contributor Author

@lloydjatkinson thanks for the reference! We original did call it fragments but switched to partials after some discussion. Your points are very valid! Going to discuss it with the core team and see what they think.

@philipschm1tt
Copy link

Just wanted to comment I find the name of the feature unclear (though I understand how the name was arrived at). Partials is already a fairly well used terminology to describe reusable templates or "views" particularly in MVC-like frameworks. These partials have little in common with this RFC. Essentially partials in this context are components.

@lloydjatkinson thanks for the reference! We original did call it fragments but switched to partials after some discussion. Your points are very valid! Going to discuss it with the core team and see what they think.

I would prefer "partials" over "fragments" in this case.

"Partials" is not uncommon (also used for things like SCSS) and not strictly about re-using – just says that it's only a part of the markup. That usage is not far off from some of the linked examples: the MVC frameworks often amount to parts of only the markup – the content of an Astro partial and something like a Java JSP "partial" would not be big.

By contrast, "fragment" is increasingly popular in the micro-frontend context (see https://engineering.zalando.com/posts/2018/12/front-end-micro-services.html or https://blog.cloudflare.com/better-micro-frontends/ ). Astro does not support that approach. The word refers more to the "outcome" of a piece of the rendered site, which also "implies" the behavior within that "fragment" and not just the markup.

An alternative term that would work: "html fragment".
It's precise: only HTML. Seems fine to me, too.

@matthewp
Copy link
Contributor Author

We are going to stick with partials, but title it as Page Partials in our documentation and communication, to specify that this is about partials that are accessible via a URL. Naming is hard and there's no perfect name. In the future if this seems like a mistake we can always rename, so this is not a blocking sort of decision.


This RFC has gone through the final comment period, I appreciate everyone's feedback!

@matthewp matthewp merged commit e51eac8 into main Oct 26, 2023
@matthewp matthewp deleted the fragments branch October 26, 2023 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants