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: build pipeline #8088

Merged
merged 8 commits into from
Aug 16, 2023
Merged

refactor: build pipeline #8088

merged 8 commits into from
Aug 16, 2023

Conversation

ematipico
Copy link
Member

Changes

This PR refactors the build process using the Pipeline class.

Here's a list of significant changes:

  • created a new plugin-manifest plugin to generate the manifest. The manifest object is now emitted in a separate file called manifest.mjs. The file is emitted in the config.build.server folder. This is an internal refactoring. I went with this change because the manifest was repeated for each emitted entry/route, giving the impression that a manifest belongs to an entry/route. This isn't correct. A manifest belongs to the whole app. It made more sense to have a separate file with this information.
  • created a new BuildPipeline class that extends Pipeline, which is responsible for giving all the essential information to its consumers; for example, BuildInternals and StaticOuputOptions are now inside BuildPipeline and with a series of getters, the consumers retrieve the information needed.
  • changed how we retrieve the pages to render without using generators. Even though it wasn't necessary, now we reduced the number of loops.

Testing

Current tests should pass.

Docs

N/A internal refactor

@changeset-bot
Copy link

changeset-bot bot commented Aug 15, 2023

⚠️ No Changeset found

Latest commit: ce393bd

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Aug 15, 2023
Comment on lines +166 to +169
const headers = new Headers();
headers.set('X-Astro-Encoding', response.encoding);
return new Response(response.body, {
headers,
});
Copy link
Member Author

Choose a reason for hiding this comment

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

I thought we could store the encoding coming from a simple response using an internal header. What do you think? When generating a page, we read it and use it when we write the file to file system.

Copy link
Member

Choose a reason for hiding this comment

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

I like it!

Copy link
Member Author

Choose a reason for hiding this comment

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

80% of the code comes from plugin-ssr.ts.

@ematipico ematipico force-pushed the refactor/build-pipeline-plt-763 branch from 48be659 to 090ac80 Compare August 15, 2023 14:25
@ematipico ematipico requested review from matthewp, bluwy and natemoo-re and removed request for matthewp August 15, 2023 14:25
Copy link
Member

@natemoo-re natemoo-re 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! Glad to see the manifest changes and a whole bunch of cleanup.

Comment on lines +166 to +169
const headers = new Headers();
headers.set('X-Astro-Encoding', response.encoding);
return new Response(response.body, {
headers,
});
Copy link
Member

Choose a reason for hiding this comment

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

I like it!

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.

Like the changes so far. Thanks for addressing my comments from the last PR!

Have some small nits below.

Comment on lines +36 to +38
const imports = [];
const contents = [];
const exports = [];
Copy link
Member

Choose a reason for hiding this comment

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

Nit: we could type this as string[]

@@ -64,6 +58,8 @@ import type {
StylesheetAsset,
} from './types';
import { getTimeStat } from './util.js';
import { BuildPipeline } from './buildPipeline.js';
import type { BufferEncoding } from 'vfile';
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think we can remove this import as BufferEncoding is a global with @types/node loaded.

@ematipico ematipico force-pushed the refactor/build-pipeline-plt-763 branch from 630d995 to 0c8134b Compare August 16, 2023 09:32
@ematipico ematipico force-pushed the refactor/build-pipeline-plt-763 branch from 0c8134b to 42ac066 Compare August 16, 2023 10:30
@ematipico ematipico requested a review from a team as a code owner August 16, 2023 10:38
@ematipico ematipico force-pushed the refactor/build-pipeline-plt-763 branch from 620e1ba to 1095ce6 Compare August 16, 2023 11:27
@ematipico ematipico force-pushed the refactor/build-pipeline-plt-763 branch from 410a18a to ce393bd Compare August 16, 2023 15:24
@ematipico ematipico merged commit ca4cf01 into next Aug 16, 2023
@ematipico ematipico deleted the refactor/build-pipeline-plt-763 branch August 16, 2023 15:45
if (typeof hashedFilePath !== 'string') {
// If no "astro:scripts/before-hydration.js" script exists in the build,
// then we can assume that no before-hydration scripts are needed.
if (specifier === BEFORE_HYDRATION_SCRIPT_ID) {
Copy link

@BlankParticle BlankParticle Sep 2, 2023

Choose a reason for hiding this comment

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

I think this check is the reason that so many hybrid render builds are failing,
in packages/src/core/build/plugins/plugin-manifest.ts:238 you are setting entryModules[BEFORE_HYDRATION_SCRIPT_ID] = '', then here you are checking if (typeof hashedFilePath !== 'string') and inside that check you are checking if (specifier === BEFORE_HYDRATION_SCRIPT_ID), but as you have already set the BEFORE_HYDRATION_SCRIPT_ID in plugin-manifest.ts so this check is never called.

as a result, while resolving BEFORE_HYDRATION_SCRIPT_ID in packages/astro/src/runtime/server/hydration.ts:157 we get "/" instead of "", for which this check passes and before-hydration-url is generated.

if (beforeHydrationUrl.length) {
		island.props['before-hydration-url'] = beforeHydrationUrl;
	}

So the potential fixes could be to change the function to be

async resolve(specifier: string) {
					//const hashedFilePath = manifest.entryModules[specifier];
        // EDIT: This one seems to work, all tests also passes
        const hashedFilePath = internals.entrySpecifierToBundleMap.get(specifier);
					if (typeof hashedFilePath !== 'string') {
					// If no "astro:scripts/before-hydration.js" script exists in the build,
					// then we can assume that no before-hydration scripts are needed.
					if (specifier === BEFORE_HYDRATION_SCRIPT_ID && hashedFilePath === "") {
						return '';
					}
						throw new Error(`Cannot find the built path for ${specifier}`);
					}
					return createAssetLink(hashedFilePath, manifest.base, manifest.assetsPrefix);
				},

or change the check at packages/astro/src/runtime/server/hydration.ts:157 to be

// Its more of a workaround than a fix
if (beforeHydrationUrl.length !== 0 && beforeHydrationUrl !== "/") {
		island.props['before-hydration-url'] = beforeHydrationUrl;
	}

I am down to make a PR to fix the issue (#8379 and many more), just waiting for confrimations from the maintainers

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.

5 participants