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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+61 −7
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d0ce475
fix(i18n): don't make virtual modules no-op
ematipico Apr 3, 2024
04a458f
fix build failure
ematipico Apr 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/short-points-hunt.md
View file
Open in desktop
@@ -0,0 +1,5 @@
---
"astro": patch
---

Fixes a case where the `i18n` virtual module was transformed into a no-op function when some functions were used inside a non-prerendered page.
2 changes: 2 additions & 0 deletions packages/astro/src/core/build/page-data.ts
View file
Open in desktop
Expand Up @@ -57,6 +57,7 @@ export async function collectPagesData(
propagatedScripts: new Map(),
hoistedScript: undefined,
hasSharedModules: false,
hasAstroVirtualModule: false,
};

clearInterval(routeCollectionLogTimeout);
Expand All @@ -82,6 +83,7 @@ export async function collectPagesData(
propagatedScripts: new Map(),
hoistedScript: undefined,
hasSharedModules: false,
hasAstroVirtualModule: false,
};
}

Expand Down
4 changes: 4 additions & 0 deletions packages/astro/src/core/build/plugins/plugin-chunks.ts
View file
Open in desktop
Expand Up @@ -12,6 +12,10 @@ export function vitePluginChunks(): VitePlugin {
if (id.includes('astro/dist/runtime/server/')) {
return 'astro/server';
}
// Split the Astro runtime into a separate chunk for readability
if (id.includes('astro/dist/runtime')) {
return 'astro';
}
},
});
},
Expand Down
15 changes: 11 additions & 4 deletions packages/astro/src/core/build/plugins/plugin-prerender.ts
View file
Open in desktop
Expand Up @@ -5,6 +5,7 @@ import type { BuildInternals } from '../internal.js';
import type { AstroBuildPlugin } from '../plugin.js';
import type { StaticBuildOptions } from '../types.js';
import { extendManualChunks } from './util.js';
import { ASTRO_VIRTUAL_MODULE } from '../../constants.js';

function vitePluginPrerender(opts: StaticBuildOptions, internals: BuildInternals): VitePlugin {
return {
Expand All @@ -13,12 +14,9 @@ function vitePluginPrerender(opts: StaticBuildOptions, internals: BuildInternals
outputOptions(outputOptions) {
extendManualChunks(outputOptions, {
after(id, meta) {
// Split the Astro runtime into a separate chunk for readability
if (id.includes('astro/dist/runtime')) {
return 'astro';
}
Comment on lines -16 to -19
Copy link
Member Author

@ematipico ematipico Apr 3, 2024 •
edited

Choose a reason for hiding this comment

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

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

const pageInfo = internals.pagesByViteID.get(id);
let hasSharedModules = false;
let hasAstroVirtualModules = false;
if (pageInfo) {
// prerendered pages should be split into their own chunk
// Important: this can't be in the `pages/` directory!
Expand All @@ -43,12 +41,20 @@ function vitePluginPrerender(opts: StaticBuildOptions, internals: BuildInternals
// Given the previous statement, we only check if
// - the module is a page, and it's not pre-rendered
// - the module is the middleware
// - the module belongs to our virtual modules, which belong to `
// If one of these conditions is met, we need a separate chunk
for (const importer of moduleMeta.importedIds) {
// we don't want to analyze the same module again, so we skip it
if (importer !== id) {
const importerModuleMeta = meta.getModuleInfo(importer);
if (importerModuleMeta) {
if (
importerModuleMeta.meta &&
importerModuleMeta.meta[ASTRO_VIRTUAL_MODULE]
) {
hasAstroVirtualModules = true;
break;
}
// if the module is inside the pages
if (importerModuleMeta.id.includes('/pages')) {
// we check if it's not pre-rendered
Expand All @@ -70,6 +76,7 @@ function vitePluginPrerender(opts: StaticBuildOptions, internals: BuildInternals

opts.allPages;
pageInfo.hasSharedModules = hasSharedModules;
pageInfo.hasAstroVirtualModule = hasAstroVirtualModules;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you have some suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@ematipico ematipico Apr 3, 2024 •
edited

Choose a reason for hiding this comment

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

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.

pageInfo.route.prerender = true;
return 'prerender';
}
Expand Down
5 changes: 4 additions & 1 deletion packages/astro/src/core/build/static-build.ts
View file
Open in desktop
Expand Up @@ -374,7 +374,10 @@ async function cleanStaticOutput(
) {
const allStaticFiles = new Set();
for (const pageData of eachPageData(internals)) {
if (pageData.route.prerender && !pageData.hasSharedModules) {
if (
pageData.route.prerender &&
!(pageData.hasSharedModules || pageData.hasAstroVirtualModule)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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' });
})
);

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

) {
const { moduleSpecifier } = pageData;
const pageBundleId = internals.pageToBundleMap.get(moduleSpecifier);
const entryBundleId = internals.entrySpecifierToBundleMap.get(moduleSpecifier);
Expand Down
4 changes: 4 additions & 0 deletions packages/astro/src/core/build/types.ts
View file
Open in desktop
Expand Up @@ -29,6 +29,10 @@ export interface PageBuildData {
hoistedScript: { type: 'inline' | 'external'; value: string } | undefined;
styles: Array<{ depth: number; order: number; sheet: StylesheetAsset }>;
hasSharedModules: boolean;
/**
* Whether the page is using astro virtual modules
*/
hasAstroVirtualModule: boolean;
}

export type AllPagesData = Record<ComponentPath, PageBuildData>;
Expand Down
18 changes: 18 additions & 0 deletions packages/astro/src/core/constants.ts
View file
Open in desktop
Expand Up @@ -71,3 +71,21 @@ export const SUPPORTED_MARKDOWN_FILE_EXTENSIONS = [

// The folder name where to find the middleware
export const MIDDLEWARE_PATH_SEGMENT_NAME = 'middleware';

/**
* This constant is used to mark internal astro virtual modules. Use this constant to attach it to the metadata of the virtual module, e.g.:
*
* ```js
* function resolveId(id) {
* if (id === virtualModuleId) {
* return {
* id,
* meta: {
* [ASTRO_VIRTUAL_MODULE]: true,
* },
* }
* }
* }
* ```
*/
export const ASTRO_VIRTUAL_MODULE = 'astro-virtual-module';
15 changes: 13 additions & 2 deletions packages/astro/src/i18n/vite-plugin-i18n.ts
View file
Open in desktop
Expand Up @@ -2,6 +2,7 @@ import type * as vite from 'vite';
import type { AstroConfig, AstroSettings } from '../@types/astro.js';
import { AstroError } from '../core/errors/errors.js';
import { AstroErrorData } from '../core/errors/index.js';
import { ASTRO_VIRTUAL_MODULE } from '../core/constants.js';

const virtualModuleId = 'astro:i18n';

Expand Down Expand Up @@ -44,10 +45,20 @@ export default function astroInternationalization({
},
};
},
resolveId(id) {
async resolveId(id) {
if (id === virtualModuleId) {
if (i18n === undefined) throw new AstroError(AstroErrorData.i18nNotEnabled);
return this.resolve('astro/virtual-modules/i18n.js');

const resolved = await this.resolve('astro/virtual-modules/i18n.js');
if (resolved) {
return {
id: resolved.id,
meta: {
[ASTRO_VIRTUAL_MODULE]: true,
},
};
}
return undefined;
}
},
};
Expand Down