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

refactor: emit pages as physical entry points #7193

Merged
merged 8 commits into from
May 25, 2023

Conversation

ematipico
Copy link
Member

@ematipico ematipico commented May 24, 2023

Changes

This PR is another internal refactor of our internal bundling strategy.

Before, the pages were bundled in a single entry point. This entry point was called entry.mjs.

With this refactor, now each page becomes an entry point.

For SSG builds, this should result in no changes whatsoever in its business logic. The build should work as it was, and the emitted files should still work as they were. The only effective change is that Astro needs to load each entry point when generating the build.

For SSR builds, this should result in an internal change. Before, all the pages were bundled inside the entry.mjs file. The pages are NOT included in the final bundle, but they are loaded via dynamic import. This is an internal change of the business logic, and the SSR build still needs to work.

Testing

The current tests MUST all pass.

Docs

I created a file inside the build/plugins/ folder. I noticed that there's some knowledge that I don't want to lose, and explaining it inside the code can be very tedious.

Unfortunately, I don't know ALL plugins, so consider this file as a starting point and a WIP. In fact, the plugin-ssr does more than I described, so probably someone else will chime in and write down something more.

cc @withastro/maintainers-docs for feedback, but I am not really sure if we need your review for internal knowledge, I don't want to give you more work than you already have! 😅

@changeset-bot
Copy link

changeset-bot bot commented May 24, 2023

🦋 Changeset detected

Latest commit: aa4959d

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 May 24, 2023
@ematipico ematipico force-pushed the refactor/pages-as-entry-points branch 2 times, most recently from 2642e53 to 7a8a394 Compare May 24, 2023 09:36
@ematipico ematipico changed the title chore: wip refactor: emit pages as physical entry points May 24, 2023
@ematipico ematipico marked this pull request as ready for review May 24, 2023 10:29
@ematipico ematipico requested a review from a team as a code owner May 24, 2023 10:29
@ematipico ematipico requested review from matthewp and bluwy May 24, 2023 10:34
@ematipico ematipico force-pushed the refactor/pages-as-entry-points branch from 051320d to 4f2aa89 Compare May 24, 2023 14:59
@github-actions github-actions bot removed the pkg: integration Related to any renderer integration (scope) label May 24, 2023
@ematipico ematipico force-pushed the refactor/pages-as-entry-points branch 2 times, most recently from be8e50a to d69e831 Compare May 24, 2023 15:36
@github-actions github-actions bot added the pkg: integration Related to any renderer integration (scope) label May 24, 2023
@ematipico ematipico force-pushed the refactor/pages-as-entry-points branch 2 times, most recently from 11bf35a to ce809c5 Compare May 24, 2023 15:40
@ematipico ematipico requested a review from matthewp May 25, 2023 08:23
@ematipico ematipico force-pushed the refactor/pages-as-entry-points branch 2 times, most recently from ed5e785 to 7935570 Compare May 25, 2023 09:24
@ematipico ematipico force-pushed the refactor/pages-as-entry-points branch from 7935570 to 37418e1 Compare May 25, 2023 09:29
packages/integrations/mdx/src/index.ts Outdated Show resolved Hide resolved
packages/astro/src/core/build/plugins/util.ts Outdated Show resolved Hide resolved
packages/astro/src/core/build/plugins/plugin-ssr.ts Outdated Show resolved Hide resolved
packages/astro/src/core/build/plugins/plugin-ssr.ts Outdated Show resolved Hide resolved
packages/astro/src/core/build/plugins/plugin-pages.ts Outdated Show resolved Hide resolved
packages/astro/src/core/build/plugins/plugin-pages.ts Outdated Show resolved Hide resolved
Comment on lines 184 to 186
} else if (chunkInfo.facadeModuleId === RESOLVED_MIDDLEWARE_MODULE_ID) {
return 'middleware.mjs';
} else if (chunkInfo.facadeModuleId === RESOLVED_RENDERERS_MODULE_ID) {
Copy link
Member

Choose a reason for hiding this comment

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

With the plugin-middleware refactor, this condition wont pass anymore. It'll still pass if you have a src/middleware.ts because the else condition here catches it. But if I have src/middleware/index.ts, it won't output middleware.mjs anymore (index.mjs instead) (tested in middleware example). Maybe it's simpler to keep the plugin-middleware changes to handle this for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! Completely missed that

Copy link
Member

Choose a reason for hiding this comment

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

@ematipico I might've pointed the wrong thing, but with the new commit, this scenario still happens:

But if I have src/middleware/index.ts, it won't output middleware.mjs anymore (index.mjs instead) (tested in middleware example).

In the middleware example with src/middleware/index.ts, the middleware is output as dist/server/index.mjs, which I don't think is intentional, and we want dist/server/middlewares.mjs instead?

I was suggesting keeping the previous plugin-middleware code, or maybe there's a property in chunkInfo that we can use to identify is a middleware?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe:

if (chunkInfo.facadeModuleId?.includes('middleware') && chunkInfo.exports.includes('onRequest')) {
  return 'middleware.mjs'
}

A bit fragile though.

Copy link
Member Author

@ematipico ematipico May 25, 2023

Choose a reason for hiding this comment

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

Updated the code!

@ematipico ematipico force-pushed the refactor/pages-as-entry-points branch from cb24b6c to 86eb4b2 Compare May 25, 2023 10:26
@github-actions github-actions bot removed the pkg: integration Related to any renderer integration (scope) label May 25, 2023
@ematipico ematipico force-pushed the refactor/pages-as-entry-points branch from 86eb4b2 to f94b638 Compare May 25, 2023 10:26
@ematipico ematipico force-pushed the refactor/pages-as-entry-points branch from f94b638 to b94d215 Compare May 25, 2023 11:15
@ematipico ematipico requested a review from bluwy May 25, 2023 11:16
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for refactoring and documenting this section of the codebase.

packages/astro/src/core/build/plugins/plugin-middleware.ts Outdated Show resolved Hide resolved
@ematipico ematipico merged commit 8b041bf into main May 25, 2023
@ematipico ematipico deleted the refactor/pages-as-entry-points branch May 25, 2023 13:28
@astrobot-houston astrobot-houston mentioned this pull request May 25, 2023
@lorenzolewis lorenzolewis mentioned this pull request Aug 3, 2023
1 task
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants