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

astro: namespace for middleware and components #8101

Merged
merged 3 commits into from
Aug 17, 2023
Merged

Conversation

matthewp
Copy link
Contributor

@matthewp matthewp commented Aug 16, 2023

Changes

  • Adds aliases of astro:middleware and astro:components

Testing

  • All updated to use the aliases

Docs

@changeset-bot
Copy link

changeset-bot bot commented Aug 16, 2023

🦋 Changeset detected

Latest commit: fdbf0c4

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: example Related to an example package (scope) pkg: astro Related to the core `astro` package (scope) semver: minor Change triggers a `minor` release labels Aug 16, 2023
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 marked this pull request as ready for review August 16, 2023 15:42
@matthewp matthewp requested a review from a team as a code owner August 16, 2023 15:42
@Princesseuh
Copy link
Member

It's a side quest not really related to this, but someone (me, probably) should make it so imports starting with astro: gets prioritized in the language-tools, otherwise when you type like <Code> it'll auto import from astro/components first

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.

About astro:middleware VS astro/middleware, here's some concerns we should take into account (even in the language tools cc @Princesseuh )

At the moment, astro/middleware exposes the following APIs: createContext, trySerializeLocals, defineMiddleware and sequence.

defineMiddleware and sequence can be part of astro:middleware, but createContext, trySerializeLocals can't be part of astro:middleware, because are runtime APIs that expose code that will be used by integrations for advanced features (e.g. edge middleware).

I wonder:

  • is it possible to exclude createContext, trySerializeLocals from astro:middleware?
  • is it possible to exclude defineMiddleware and sequence from astro/middleware?
  • is it possible to suggest the correct import based on the context? Probably yes, because the middleware is a known file with fixed location and name.

---


`astro:`namespace aliases for middleware and components
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`astro:`namespace aliases for middleware and components
`astro:` namespace aliases for middleware and components

@bluwy
Copy link
Member

bluwy commented Aug 17, 2023

but createContext, trySerializeLocals can't be part of astro:middleware, because are runtime APIs that expose code that will be used by integrations for advanced features (e.g. edge middleware).

Couldn't createContext and trySerializeLocals still be part of astro:middleware, but integrations can import from astro/middleware instead? They'd be unused in the other environments, but I think it's probably fine so it's easier to maintain.

@matthewp
Copy link
Contributor Author

It's easy enough to create another module to use as the alias so I'll do that.

@ematipico
Copy link
Member

but createContext, trySerializeLocals can't be part of astro:middleware, because are runtime APIs that expose code that will be used by integrations for advanced features (e.g. edge middleware).

Couldn't createContext and trySerializeLocals still be part of astro:middleware, but integrations can import from astro/middleware instead? They'd be unused in the other environments, but I think it's probably fine so it's easier to maintain.

As long as the functions are correctly bundled by esbuild, it's fine by me.

@matthewp matthewp merged commit ea7ff51 into next Aug 17, 2023
14 checks passed
@matthewp matthewp deleted the astro-namespace branch August 17, 2023 13:28
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.

None yet

5 participants