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

feat: remove experimental flag middleware #7109

Merged
merged 17 commits into from
Jun 5, 2023
Merged

Conversation

ematipico
Copy link
Member

@ematipico ematipico commented May 17, 2023

Changes

This PR removes the experimental flag for the middleware.

While doing so, I had to change how types work inside the manifest. The middleware is a piece of code that is shared among all pages/entry points, but this doesn't work very well in SSR.

The way the SSR bundling logic works is logic this:

  1. the plugin plugin-pages.ts generates a virtual module that looks like this export { middleware, page, renderers } and middleware is imported from a shared chunk.
  2. the plugin plugin-ssr tries to import all the virtual modules from the previous plugin, and then tries to take the middleware code and expose it via manifest. Doing so could work better with the current plugin architecture.

So, to ensure that the code generated works with the current runtime, I removed the middleware variable from the manifest. This is safe because the virtual page emitted by plugin-pages.ts also has the middleware code.

This change made me realise that the current architecture is brittle and confusing, although I already know how I can change it. I will follow up with different PRs to make the change less confusing.

Testing

Current tests should all pass.

The examples should be updated after this PR lands.

Docs

N/A

@changeset-bot
Copy link

changeset-bot bot commented May 17, 2023

🦋 Changeset detected

Latest commit: 14cfed1

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 pkg: astro Related to the core `astro` package (scope) pkg: example Related to an example package (scope) semver: minor Change triggers a `minor` release labels May 17, 2023
@github-actions
Copy link
Contributor

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

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

Block until 2.6

@ematipico ematipico force-pushed the fix/middleware-for-endpoints branch from d8ca589 to 4217953 Compare May 23, 2023 13:59
@ematipico ematipico marked this pull request as ready for review May 25, 2023 13:41
@ematipico ematipico requested a review from a team as a code owner May 25, 2023 13:41
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.

Just a note below re: removing middleware from the list of experimental flags in docs!

packages/astro/src/@types/astro.ts Show resolved Hide resolved
@ematipico ematipico force-pushed the fix/middleware-for-endpoints branch from 62a524d to f6072df Compare May 30, 2023 12:04
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

@matthewp matthewp dismissed their stale review June 5, 2023 13:30

Merge day

@matthewp matthewp merged commit 101f032 into main Jun 5, 2023
@matthewp matthewp deleted the fix/middleware-for-endpoints branch June 5, 2023 15:25
@astrobot-houston astrobot-houston mentioned this pull request Jun 5, 2023
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: example Related to an example package (scope) semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants