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: expose middleware URL to integrations #7458

Merged

Conversation

ematipico
Copy link
Member

@ematipico ematipico commented Jun 23, 2023

Changes

This is another feature towards the support of Vercel Edge Middleware.

This feature exposes the file path of the middleware file emitted during the astro build.

Testing

I extended the functionality of the test adapter and created a test that makes sure that the file exists and it has content.

Docs

I will document the new payload towards the end of the feature.

@changeset-bot
Copy link

changeset-bot bot commented Jun 23, 2023

🦋 Changeset detected

Latest commit: 5462af5

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: integration Related to any renderer integration (scope) labels Jun 23, 2023
@ematipico ematipico force-pushed the feat/vercel/expose-middleware-entrypoint branch from 8eee9e1 to 1375192 Compare June 23, 2023 08:23
@ematipico ematipico changed the title wip feat: expose middleware URL to integrations Jun 23, 2023
@ematipico ematipico force-pushed the feat/vercel/expose-middleware-entrypoint branch from 1375192 to c62ecfa Compare June 23, 2023 08:26
@ematipico ematipico marked this pull request as ready for review June 23, 2023 08:33
@ematipico ematipico requested a review from a team as a code owner June 23, 2023 08:33
@github-actions github-actions bot added the semver: minor Change triggers a `minor` release label Jun 23, 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.

if (chunk.type === 'asset') {
continue;
}
if (chunk.fileName === 'middleware.mjs') {
Copy link
Member Author

@ematipico ematipico Jun 23, 2023

Choose a reason for hiding this comment

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

I will put this string in a shared variable in the following PRs

Copy link
Member

@ElianCodes ElianCodes left a comment

Choose a reason for hiding this comment

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

Awesome! Just left a small formatting nit

packages/astro/src/core/build/internal.ts Outdated Show resolved Hide resolved
packages/astro/test/middleware.test.js Outdated Show resolved Hide resolved
.changeset/chilly-pants-fix.md Outdated Show resolved Hide resolved
continue;
}
if (chunk.fileName === 'middleware.mjs') {
internals.middlewareEntryPoint = new URL(chunkName, opts.settings.config.outDir);
Copy link
Member

Choose a reason for hiding this comment

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

Should we combine outDir + "server" + chunkName here so we don't have to handle constructing the final middlewareEntryPoint again incore/build/index.ts? Otherwise this URL path would also point to a non existent path.

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically, at this point of the execution, the path is correct. The files are moved later. If we decided to prefix "server" here, we would create a URL that point to a file that doesn't exist. If, in the future, we decide to expose this information to a hook that is called before the files are moved, we could incur an error.

Copy link
Member

Choose a reason for hiding this comment

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

That's fair 👍 I'm mostly concerned that the final constructed path takes a lot of work to do so, which might not be great for perf. I won't let this block the PR though, I'm also fine with this for now.

@ematipico ematipico force-pushed the feat/vercel/expose-middleware-entrypoint branch from 72fdbfb to fabe9ab Compare June 23, 2023 14:35
@ematipico ematipico requested a review from bluwy June 23, 2023 14:35
@ematipico ematipico force-pushed the feat/vercel/expose-middleware-entrypoint branch 2 times, most recently from d6d7076 to 7d1395a Compare June 23, 2023 14:37
@bluwy
Copy link
Member

bluwy commented Jun 26, 2023

Looks like there's a CI fail, but the direction is good for me.

@ematipico ematipico force-pushed the feat/vercel/expose-middleware-entrypoint branch from 7d1395a to 5462af5 Compare June 26, 2023 12:01
@ematipico ematipico merged commit 26fce13 into feat/vercel-edge-middleware Jun 26, 2023
@ematipico ematipico deleted the feat/vercel/expose-middleware-entrypoint branch June 26, 2023 12:01
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: integration Related to any renderer integration (scope) semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants