Skip to content

fix(build): generated entry#12477

Merged
ematipico merged 1 commit intomainfrom
fix/middleware-import-ssr
Nov 19, 2024
Merged

fix(build): generated entry#12477
ematipico merged 1 commit intomainfrom
fix/middleware-import-ssr

Conversation

@ematipico
Copy link
Copy Markdown
Member

@ematipico ematipico commented Nov 19, 2024 •
edited
Loading

Changes

The middleware is now dynamically imported, which means we don't need to have it at the top of entry.mjs

Closes PLT-2659

Testing

I tested locally and checked the emitted file. That import is unused, so there's no need for new tests.

Current CI should pass

Docs

N/A

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Nov 19, 2024

🦋 Changeset detected

Latest commit: c548eb2

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 the pkg: astro Related to the core `astro` package (scope) label Nov 19, 2024
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Nov 19, 2024 •
edited
Loading

CodSpeed Performance Report

Merging #12477 will degrade performances by 11.86%

Comparing fix/middleware-import-ssr (c548eb2) with main (733d6c1)

Summary

❌ 1 (👁 1) regressions
✅ 5 untouched benchmarks

Benchmarks breakdown

Benchmark main fix/middleware-import-ssr Change
👁 Rendering: streaming [true], .mdx file 1 s 1.2 s -11.86%

@florian-lefebvre
Copy link
Copy Markdown
Member

florian-lefebvre commented Nov 19, 2024

Any reason why changes about globalThis are not in this PR?

@ematipico
Copy link
Copy Markdown
Member Author

ematipico commented Nov 19, 2024 •
edited
Loading

Any reason why changes about globalThis are not in this PR?

This fix has to be shipped regardless of astro:env. We generate an incorrect code nonetheless. Myself and Matthew agreed that we can ship this fix while we wait for a good fix for astro env

florian-lefebvre
florian-lefebvre reviewed Nov 19, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@florian-lefebvre florian-lefebvre left a comment

Choose a reason for hiding this comment

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

This looks good but I think we should also get rid of the generated middleware.mjs file, as I did in #12326

@ematipico
Copy link
Copy Markdown
Member Author

ematipico commented Nov 19, 2024

I'll submit a new PR if that's OK with you @florian-lefebvre

@ematipico ematipico merged commit 46f6b38 into main Nov 19, 2024
@ematipico ematipico deleted the fix/middleware-import-ssr branch November 19, 2024 16:12
This was referenced Nov 19, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@florian-lefebvre florian-lefebvre florian-lefebvre approved these changes

Assignees

No one assigned

Labels

pkg: astro Related to the core `astro` package (scope)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ematipico @florian-lefebvre