fix(i18n): don't make virtual modules no-op

[ematipico]

Changes

Closes #10620
Closes PLT-1920

This was another tricky issue to fix. The plugin-prerender plugin was marking computePreferredLocale as a no-op function.

In order to prevent that, I decided to go for a more generic route, in case something like this happens in the future. I created a new constant that we can use to "mark" our virtual modules as "Astro internal virtual modules", and these virtual modules won't be transformed by the plugin.

As for now, I marked the i18n plugin as internal. Not sure if we need to do this also for others.

Testing

I tested this one locally, using the repository provided by OP. It works as expected now.

Docs

N/A

[changeset-bot]

🦋 Changeset detected

Latest commit: 04a458f

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

[ematipico]

It's NOT part of the fix, but having this is here felt very out of place, and I moved it inside plugin-chunks

[matthewp]

is this part the actual fix of the issue? Can you explain why this is needed if so?

[ematipico]

Yes this is part of the fix. This is where we collect the chunks, few lines below we clean them up by making them no-op:

astro/packages/astro/src/core/build/static-build.ts

Lines 397 to 413 in 317d18e

	await Promise.all(
	files.map(async (filename) => {
	if (!allStaticFiles.has(filename)) {
	return;
	}
	const url = new URL(filename, out);
	const text = await fs.promises.readFile(url, { encoding: 'utf8' });
	const [, exports] = eslexer.parse(text);
	// Replace exports (only prerendered pages) with a noop
	let value = 'const noop = () => {};';
	for (const e of exports) {
	if (e.n === 'default') value += `\n export default noop;`;
	else value += `\nexport const ${e.n} = noop;`;
	}
	await fs.promises.writeFile(url, value, { encoding: 'utf8' });
	})
	);

[matthewp]

Ok got it. It's a little confusing that this prerender specific logic is inlined into the build like this. What do you think about moving this to it's own function or something to improve readability?

[matthewp]

Looks like this property is only set when there is prerendering, is that right? If so there might be a better name for this, so people don't think it can be used elsewhere.

[ematipico]

Do you have some suggestions?

[matthewp]

Not sure yet, what is the reason why prerendering needs special handling for virtual modules?

[ematipico]

That's the other way around, virtual modules need special treatment for prerendered pages.

It seems that the static build takes the chunk that contains the virtual module and makes it a no-op.

I'm not very familiar with the specifics of business logic though.

[bluwy]

I'm not really sure if this is the best fix for the issue. If there is ASTRO_VIRTUAL_MODULE to mark virtual modules, why is it only added for the astro:i18n virtual module. And I also don't think virtual modules should have to manually mark this.

I don't understand the issue well to suggest another fix, but I think there would be a different "bigger-picture" fix to prevent the no-op in the first place.

[ematipico]

@bluwy

All your concerns are valid, as they are my very own concerns. I was hoping that you guys had more context and knowledge about the prerendering logic and how the no-op strategy works.

Not sure if you checked the source code of plugin-prerenderer.ts, but there's also some dirty logic where modules that were used in the middleware AND a pre-rendered page were made no-op. So the logic is checks around checking the middleware file.

This issue is even worse because the functions that are made no-op are functions inside the Astro core.

[natemoo-re]

Great discussion @bluwy and @ematipico! Totally agree that this original prerendering logic has not held up as Astro's capabilities have grown. This was all written shortly after we initially added SSR support, so things like middleware, virtual modules, and our more complex bundling setup were never properly accounted for.

If we're looking for a long-term refactor, I have some context to share on how and why things work as they currently do. Our build process is currently one single Rollup build (via Vite). For features like prerender and content collection caching, we are hitting the limits of manually mutating specific chunks before/after that build process.

The prerender noop replacement was alway a hacky approach intended to avoid a secondary build step just for prerendered pages. It has become increasingly apparent that we would reduce complexity by separating prerendered pages from dynamically rendered ones, so that there are no shared chunks that need special logic like this PR adds.

The main challenges I foresee with this multi-build approach are that:

1. All routes must be compiled in order to determine which ones are prerendered vs dynamically rendered. Not 100% sure how to pull that off.
2. Certain moudles/logic will likely be duplicated when sent to two separate Rollup builds. Seems fine, tbh!

Those tradeoffs and challenge still seem the most correct, given the number of edge cases we've had to fix with the current approach. As part of these refactors, we could even unify the divergent logic for static/hybrid/server modes since they're all essentially the same? That would definitely be a maintainability win.

[lilnasy]

• Are you suggesting splitting the production build into two manifests - one containing only the routeData for prerendered pages, one only for on-demand rendered pages?

• Could you elaborate on "unifying" logic for static, hybrid, and server modes?

[natemoo-re]

@lilnasy good questions! The manifest generation is a big open question. But yes, the gist of my recommendation would be to run pre-rendered pages in one bundler process and on-demand pages in another. The other idea would be to disable Rollup’s chunking behavior entirely for the first stage to avoid any shared chunks, then bundle just the code for the routes that should be rendered on-demand.

As for “unifying” the logic between build modes, I only mean internally. The API would remain the same. We currently have quite a few conditional branches that use an isServerLike() check. We probably could eliminate most of them, since “static” mode is really just “server” + all pre-rendered.