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

feature(astrojs/cloudflare): add support for splitted SSR bundles #7464

Merged
merged 20 commits into from
Jun 30, 2023
Merged

feature(astrojs/cloudflare): add support for splitted SSR bundles #7464

merged 20 commits into from
Jun 30, 2023

Conversation

alexanderniebuhr
Copy link
Member

@alexanderniebuhr alexanderniebuhr commented Jun 23, 2023

Changes

DEMO: https://astro-ssr-split-directory.pages.dev

Tasks

Preview Give feedback

Testing

  • added test suite & manually with cloudflare pages demo

Docs

/cc @withastro/maintainers-docs for feedback!

@changeset-bot
Copy link

changeset-bot bot commented Jun 23, 2023

🦋 Changeset detected

Latest commit: aaf1ff6

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

@alexanderniebuhr alexanderniebuhr changed the title DRAFT: WIP support SSR split in astrojs/cloudflare DRAFT WIP support SSR split in astrojs/cloudflare Jun 23, 2023
@github-actions github-actions bot added the pkg: integration Related to any renderer integration (scope) label Jun 23, 2023
@alexanderniebuhr alexanderniebuhr changed the title DRAFT WIP support SSR split in astrojs/cloudflare feature(astrojs/cloudflare): add experimental support for splitted SSR bundles Jun 25, 2023
@alexanderniebuhr
Copy link
Member Author

@ematipico first of all, sorry for pinging. But I would like to get this one out as soon as possible; I think having experimental support is in any way better than having no support! Iteration in the future can improve the support.

Not sure if you are the correct one to review, but if you can help with getting this on track would be amazing.

@alexanderniebuhr alexanderniebuhr marked this pull request as ready for review June 25, 2023 17:52
@alexanderniebuhr alexanderniebuhr requested a review from a team as a code owner June 25, 2023 17:52
@alexanderniebuhr

This comment was marked as resolved.

@matthewp
Copy link
Contributor

This looks great! Left a couple of comments on the code, but overall like the implementation.

Just so I understand, this is not toggling split on at the integration, but rather respecting the user's split value, right?

@alexanderniebuhr
Copy link
Member Author

alexanderniebuhr commented Jun 26, 2023

Just so I understand, this is not toggling split on at the integration, but rather respecting the user's split value, right?

Yes this is totally right. It just makes current cloudflare adapter work with the following user options (while before it was just breaking):

adapter: cloudflare({ mode: 'directory' }),
build: {
    split: true,
    excludeMiddleware: true
},

@alexanderniebuhr

This comment was marked as outdated.

@matthewp
Copy link
Contributor

matthewp commented Jun 26, 2023

Hey @alexanderniebuhr This is looking great! Have been talking about the astro:build:ssr having incorrect paths with @ematipico and we think we need to fix that issue first so that adapters don't need to deal with it.

Leaving a note here on that issue just because I don't think there's a better place. @ematipico in my investigation what I see is:

  1. In astro:build:ssr these paths are wrong but the real files do exist inside of build.server.
    • This makes me think that it's a matter of the wrong paths being saved in internals.
  2. Also of note, the real files are hashed filenames that are later renamed to the "pretty names" here:
    const fileURL = new URL(fileName, root);
    await fs.promises.mkdir(new URL('./', fileURL), { recursive: true });
    await fs.promises.writeFile(fileURL, mutation.code, 'utf-8');
    • Should they be renamed to the pretty names or should they rename the hash names?
    • If we need/want the prettier names, then we need to make it so that the astro:build:ssr hook runs after this code. Currently it does not.

@alexanderniebuhr

This comment was marked as outdated.

@ematipico
Copy link
Member

!preview cloudflare-split

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pkg: create-astro Related to the `create-astro` package (scope) pkg: example Related to an example package (scope) pkg: lit Related to Lit (scope) labels Jun 27, 2023
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.

The first round of code review. It looks good to me, nothing major from my point of view, and the tests seem solid.

Let's wait for the docs team

packages/integrations/cloudflare/src/index.ts Outdated Show resolved Hide resolved
packages/integrations/cloudflare/src/index.ts Outdated Show resolved Hide resolved
packages/integrations/cloudflare/src/index.ts Show resolved Hide resolved
@ematipico
Copy link
Member

By the way, @alexanderniebuhr , this contribution of yours is fantastic! Great work, and thank you! 🚀

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Thanks for this helpful contribution, @alexanderniebuhr! I'm sure people are going to appreciate this!

Left some suggestions for your consideration based on:

  • Is this actually "experimental" (i.e. behind a flag?) vs just "new"?
  • Changesets are now being used to autogenerate blog post material, so congrats, this PR is suffering through those growing pains of changeset editing, not just README editing! 😄
  • suggestion to maybe not refer to the optional config so early on, front-loading an already kind of big/explainy paragraph of the default behavior, and letting the 2nd paragraph do the heavy lifting?

.changeset/healthy-books-study.md Outdated Show resolved Hide resolved
packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved
packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved
@alexanderniebuhr alexanderniebuhr changed the title feature(astrojs/cloudflare): add experimental support for splitted SSR bundles feature(astrojs/cloudflare): add support for splitted SSR bundles Jun 28, 2023
Co-authored-by: Sarah Rainsberger <[email protected]>
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Docs is happy! 🥳

@ematipico ematipico merged commit 1a59185 into withastro:main Jun 30, 2023
@astrobot-houston astrobot-houston mentioned this pull request Jun 30, 2023
matthewp pushed a commit that referenced this pull request Jul 11, 2023
…7464)

* initial commit

* try to fix windows

* output files directly into the correct folder

* allow for rest parameters

* use fixed hook

* improve tests

* apply doc's team suggestions for README

Co-authored-by: Sarah Rainsberger <[email protected]>

* try to fix prerendering

* apply doc's team suggestion for changeset

Co-authored-by: Sarah Rainsberger <[email protected]>

* bump to minor

* readme update

* resolve review comments

* optimize memory allocation

* resolve review comments

* add removed link, to make sure old docs keep same

* resolve comment

Co-authored-by: Sarah Rainsberger <[email protected]>

---------

Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Chris Swithinbank <[email protected]>
@alexanderniebuhr alexanderniebuhr deleted the alexanderniebuhr/cloudflare-split-functions branch September 23, 2023 05:37
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SSR split does not work with Cloudflare
5 participants