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

Simplify HMR handling #5811

Merged
merged 10 commits into from
Jan 11, 2023
Merged

Simplify HMR handling #5811

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5d9ea1f
Simplify HMR handling
bluwy Jan 10, 2023
e30d6f2
Try skip test to reveal other test result
bluwy Jan 10, 2023
02ae325
Support virtual files
bluwy Jan 10, 2023
a451570
Fix head injection
bluwy Jan 10, 2023
2baba5b
Revert CI changes
bluwy Jan 10, 2023
2b17d90
Merge branch 'main' into try-simplify-hmr
bluwy Jan 11, 2023
c4d2cb0
Bring back normalizeFilename
bluwy Jan 11, 2023
f7ccfe6
Refactor
bluwy Jan 11, 2023
964bb25
Merge branch 'main' into try-simplify-hmr
bluwy Jan 11, 2023
b005ea6
Add changeset
bluwy Jan 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions packages/astro/src/core/compile/compile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ export interface CompileProps {
astroConfig: AstroConfig;
viteConfig: ResolvedConfig;
filename: string;
id: string | undefined;
source: string;
}

Expand All @@ -25,7 +24,6 @@ export async function compile({
astroConfig,
viteConfig,
filename,
id: moduleId,
source,
}: CompileProps): Promise<CompileResult> {
const cssDeps = new Set<string>();
Expand All @@ -37,7 +35,7 @@ export async function compile({
// use `sourcemap: "both"` so that sourcemap is included in the code
// result passed to esbuild, but also available in the catch handler.
transformResult = await transform(source, {
moduleId,
moduleId: filename,
pathname: filename,
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 found the filename and the moduleId passed are always the same (absolute paths or virtual ids). They could only differ in the resolveId hook, but since we're compiling in the transform hook, it doesn't affect us.

In a future compiler PR, I'll merge moduleId and pathname options into one.

Copy link
Contributor

Choose a reason for hiding this comment

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

So filename is the module ID right? To do head propagation we rely on the moduleId being passed to the compiler to match what is found in the module graph. Just want to make sure that is still going to be happening here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, I tested it and saw it's always identical, even for virtual files. I think the unit test should cover if not as I was able to get it to fail when I pass the wrong id during manual testing.

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 did another quick review, and I think this part I guess I'm slightly unsure:

astro/packages/astro/src/vite-plugin-astro/index.ts

Lines 244 to 245 in 8404121

filename: context.file,
id: context.modules[0]?.id ?? undefined,

When you added the context.modules[0]?.id, is it because it differs from context.file? If I understand the module graph right, Vite's code should have them equal in most case.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I did that out of "correctness" rather than a concern about context.file. So I think we can go with the change for now and see what happens.

projectRoot: astroConfig.root.toString(),
site: astroConfig.site?.toString(),
Expand Down
2 changes: 1 addition & 1 deletion packages/astro/src/core/create-vite.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ export async function createVite(
astroIntegrationsContainerPlugin({ settings, logging }),
astroScriptsPageSSRPlugin({ settings }),
astroHeadPropagationPlugin({ settings }),
astroScannerPlugin({ settings, logging }),
astroScannerPlugin({ settings }),
astroInjectEnvTsPlugin({ settings, logging, fs }),
...(settings.config.experimental.contentCollections
? [
Expand Down
13 changes: 7 additions & 6 deletions packages/astro/src/vite-plugin-astro/compile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import { getFileInfo } from '../vite-plugin-utils/index.js';

interface CachedFullCompilation {
compileProps: CompileProps;
rawId: string;
logging: LogOptions;
}

Expand All @@ -27,7 +26,6 @@ const FRONTMATTER_PARSE_REGEXP = /^\-\-\-(.*)^\-\-\-/ms;

export async function cachedFullCompilation({
compileProps,
rawId,
logging,
}: CachedFullCompilation): Promise<FullCompileResult> {
let transformResult: CompileResult;
Expand All @@ -37,7 +35,7 @@ export async function cachedFullCompilation({
transformResult = await cachedCompilation(compileProps);
// Compile all TypeScript to JavaScript.
// Also, catches invalid JS/TS in the compiled output before returning.
esbuildResult = await transformWithEsbuild(transformResult.code, rawId, {
esbuildResult = await transformWithEsbuild(transformResult.code, compileProps.filename, {
loader: 'ts',
target: 'esnext',
sourcemap: 'external',
Expand All @@ -51,15 +49,18 @@ export async function cachedFullCompilation({
} catch (err: any) {
await enhanceCompileError({
err,
id: rawId,
id: compileProps.filename,
source: compileProps.source,
config: compileProps.astroConfig,
logging: logging,
});
throw err;
}

const { fileId: file, fileUrl: url } = getFileInfo(rawId, compileProps.astroConfig);
const { fileId: file, fileUrl: url } = getFileInfo(
compileProps.filename,
compileProps.astroConfig
);

let SUFFIX = '';
SUFFIX += `\nconst $$file = ${JSON.stringify(file)};\nconst $$url = ${JSON.stringify(
Expand All @@ -70,7 +71,7 @@ export async function cachedFullCompilation({
if (!compileProps.viteConfig.isProduction) {
let i = 0;
while (i < transformResult.scripts.length) {
SUFFIX += `import "${rawId}?astro&type=script&index=${i}&lang.ts";`;
SUFFIX += `import "${compileProps.filename}?astro&type=script&index=${i}&lang.ts";`;
i++;
}
}
Expand Down
104 changes: 23 additions & 81 deletions packages/astro/src/vite-plugin-astro/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,13 @@ import type { AstroSettings } from '../@types/astro';
import type { LogOptions } from '../core/logger/core.js';
import type { PluginMetadata as AstroPluginMetadata } from './types';

import slash from 'slash';
import { fileURLToPath } from 'url';
import { normalizePath } from 'vite';
import { cachedCompilation, CompileProps, getCachedCompileResult } from '../core/compile/index.js';
import {
isRelativePath,
prependForwardSlash,
removeLeadingForwardSlashWindows,
startsWithForwardSlash,
} from '../core/path.js';
import { viteID } from '../core/util.js';
import { normalizeFilename } from '../vite-plugin-utils/index.js';
import { isRelativePath } from '../core/path.js';
import { cachedFullCompilation } from './compile.js';
import { handleHotUpdate } from './hmr.js';
import { parseAstroRequest, ParsedRequestResult } from './query.js';
import { parseAstroRequest } from './query.js';
import { normalizeFilename } from '../vite-plugin-utils/index.js';
export { getAstroMetadata } from './metadata.js';
export type { AstroPluginMetadata };

Expand All @@ -27,85 +20,28 @@ interface AstroPluginOptions {
}

/** Transform .astro files for Vite */
export default function astro({ settings, logging }: AstroPluginOptions): vite.Plugin {
export default function astro({ settings, logging }: AstroPluginOptions): vite.Plugin[] {
const { config } = settings;
let resolvedConfig: vite.ResolvedConfig;

// Variables for determining if an id starts with /src...
const srcRootWeb = config.srcDir.pathname.slice(config.root.pathname.length - 1);
const isBrowserPath = (path: string) => path.startsWith(srcRootWeb) && srcRootWeb !== '/';
const isFullFilePath = (path: string) =>
path.startsWith(prependForwardSlash(slash(fileURLToPath(config.root))));

function relativeToRoot(pathname: string) {
const arg = startsWithForwardSlash(pathname) ? '.' + pathname : pathname;
const url = new URL(arg, config.root);
return slash(fileURLToPath(url)) + url.search;
}

function resolveRelativeFromAstroParent(id: string, parsedFrom: ParsedRequestResult): string {
const filename = normalizeFilename(parsedFrom.filename, config);
const resolvedURL = new URL(id, `file://${filename}`);
const resolved = resolvedURL.pathname;
if (isBrowserPath(resolved)) {
return relativeToRoot(resolved + resolvedURL.search);
}
return slash(fileURLToPath(resolvedURL)) + resolvedURL.search;
}

return {
const prePlugin: vite.Plugin = {
name: 'astro:build',
enforce: 'pre', // run transforms before other plugins can
configResolved(_resolvedConfig) {
resolvedConfig = _resolvedConfig;
},
// note: don’t claim .astro files with resolveId() — it prevents Vite from transpiling the final JS (import.meta.glob, etc.)
async resolveId(id, from, opts) {
Comment on lines -62 to -63
Copy link
Member Author

Choose a reason for hiding this comment

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

This entire resolveId hook is removed in favour of the next review comment.

// If resolving from an astro subresource such as a hoisted script,
// we need to resolve relative paths ourselves.
if (from) {
const parsedFrom = parseAstroRequest(from);
const isAstroScript = parsedFrom.query.astro && parsedFrom.query.type === 'script';
if (isAstroScript && isRelativePath(id)) {
return this.resolve(resolveRelativeFromAstroParent(id, parsedFrom), from, {
custom: opts.custom,
skipSelf: true,
});
}
}

// serve sub-part requests (*?astro) as virtual modules
const { query } = parseAstroRequest(id);
if (query.astro) {
// TODO: Try to remove these custom resolve so HMR is more predictable.
// Convert /src/pages/index.astro?astro&type=style to /Users/name/
// Because this needs to be the id for the Vite CSS plugin to property resolve
// relative @imports.
if (query.type === 'style' && isBrowserPath(id)) {
return relativeToRoot(id);
}
// Strip `/@fs` from linked dependencies outside of root so we can normalize
// it in the condition below. This ensures that the style module shared the same is
// part of the same "file" as the main Astro module in the module graph.
// "file" refers to `moduleGraph.fileToModulesMap`.
if (query.type === 'style' && id.startsWith('/@fs')) {
id = removeLeadingForwardSlashWindows(id.slice(4));
}
// Convert file paths to ViteID, meaning on Windows it omits the leading slash
if (isFullFilePath(id)) {
return viteID(new URL('file://' + id));
}
return id;
}
},
async load(id, opts) {
const parsedId = parseAstroRequest(id);
const query = parsedId.query;
if (!query.astro) {
return null;
}
// For CSS / hoisted scripts, the main Astro module should already be cached
const filename = normalizeFilename(parsedId.filename, config);
Copy link
Member Author

Choose a reason for hiding this comment

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

This still needs a normalizeFilename in load as it may receive a root-relative path, like /src/components/Foo.astro. This seems to only happen in tests (likely the mocked fs used?), but I leave it here for now just in case.

const filename = normalizePath(normalizeFilename(parsedId.filename, config.root));
const compileResult = getCachedCompileResult(config, filename);
if (!compileResult) {
return null;
Expand Down Expand Up @@ -152,7 +88,7 @@ export default function astro({ settings, logging }: AstroPluginOptions): vite.P
if (src.startsWith('/') && !isBrowserPath(src)) {
const publicDir = config.publicDir.pathname.replace(/\/$/, '').split('/').pop() + '/';
throw new Error(
`\n\n<script src="${src}"> references an asset in the "${publicDir}" directory. Please add the "is:inline" directive to keep this asset from being bundled.\n\nFile: ${filename}`
`\n\n<script src="${src}"> references an asset in the "${publicDir}" directory. Please add the "is:inline" directive to keep this asset from being bundled.\n\nFile: ${id}`
);
}
}
Expand Down Expand Up @@ -196,20 +132,14 @@ export default function astro({ settings, logging }: AstroPluginOptions): vite.P
return;
}

const filename = normalizeFilename(parsedId.filename, config);
Copy link
Member Author

Choose a reason for hiding this comment

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

In other diffs, you'll see that I had removed normalizeFilename. That's because normalizeFilename was previously used to emulate Vite's resolver algorithm, and only needed in a pre plugins resolveId.

Since this is a transform hook, we can remove it as it's already normalized by Vite.

const compileProps: CompileProps = {
astroConfig: config,
viteConfig: resolvedConfig,
filename,
id,
filename: normalizePath(parsedId.filename),
source,
};

const transformResult = await cachedFullCompilation({
compileProps,
rawId: id,
logging,
});
const transformResult = await cachedFullCompilation({ compileProps, logging });

for (const dep of transformResult.cssDeps) {
this.addWatchFile(dep);
Expand Down Expand Up @@ -242,7 +172,6 @@ export default function astro({ settings, logging }: AstroPluginOptions): vite.P
astroConfig: config,
viteConfig: resolvedConfig,
filename: context.file,
id: context.modules[0]?.id ?? undefined,
source: await context.read(),
};
const compile = () => cachedCompilation(compileProps);
Expand All @@ -253,6 +182,19 @@ export default function astro({ settings, logging }: AstroPluginOptions): vite.P
});
},
};

const normalPlugin: vite.Plugin = {
name: 'astro:build:normal',
resolveId(id) {
// If Vite resolver can't resolve the Astro request, it's likely a virtual Astro file, fallback here instead
const parsedId = parseAstroRequest(id);
if (parsedId.query.astro) {
return id;
}
},
};
Comment on lines +186 to +195
Copy link
Member Author

Choose a reason for hiding this comment

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

... Continuing. This resolveId hook should cover all our cases instead.

The reason we can remove the pre plugin resolveId completely is because the paths can already be resolved by Vite's resolver plugin. Pre plugins run before Vite's resolver. Previously we try to resolve it ourselves, and the edge cases compound because we had to emulate the Vite resolver algorithm to get the flow right (which isn't feasible)

Now we let Vite handle instead, and only catch those that don't work with a normal plugin, which runs after Vite's resolver.


return [prePlugin, normalPlugin];
}

function appendSourceMap(content: string, map?: string) {
Expand Down
14 changes: 3 additions & 11 deletions packages/astro/src/vite-plugin-scanner/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,18 @@
import { Plugin as VitePlugin } from 'vite';
import { normalizePath, Plugin as VitePlugin } from 'vite';
import { AstroSettings } from '../@types/astro.js';
import type { LogOptions } from '../core/logger/core.js';
import { isEndpoint, isPage } from '../core/util.js';
import { normalizeFilename } from '../vite-plugin-utils/index.js';

import { scan } from './scan.js';

export default function astroScannerPlugin({
settings,
logging,
}: {
settings: AstroSettings;
logging: LogOptions;
}): VitePlugin {
export default function astroScannerPlugin({ settings }: { settings: AstroSettings }): VitePlugin {
return {
name: 'astro:scanner',
enforce: 'post',

async transform(this, code, id, options) {
if (!options?.ssr) return;

const filename = normalizeFilename(id, settings.config);
const filename = normalizePath(id);
let fileURL: URL;
try {
fileURL = new URL(`file://${filename}`);
Expand Down
5 changes: 2 additions & 3 deletions packages/astro/src/vite-plugin-scripts/page-ssr.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import MagicString from 'magic-string';
import { Plugin as VitePlugin } from 'vite';
import { normalizePath, Plugin as VitePlugin } from 'vite';
import { AstroSettings } from '../@types/astro.js';
import { isPage } from '../core/util.js';
import { normalizeFilename } from '../vite-plugin-utils/index.js';
import { PAGE_SSR_SCRIPT_ID } from './index.js';

export default function astroScriptsPostPlugin({
Expand All @@ -19,7 +18,7 @@ export default function astroScriptsPostPlugin({
const hasInjectedScript = settings.scripts.some((s) => s.stage === 'page-ssr');
if (!hasInjectedScript) return;

const filename = normalizeFilename(id, settings.config);
const filename = normalizePath(id);
let fileURL: URL;
try {
fileURL = new URL(`file://${filename}`);
Expand Down
7 changes: 4 additions & 3 deletions packages/astro/src/vite-plugin-utils/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { fileURLToPath } from 'url';
import ancestor from 'common-ancestor-path';
import type { AstroConfig } from '../@types/astro';
import {
Expand Down Expand Up @@ -44,11 +45,11 @@ export function getFileInfo(id: string, config: AstroConfig) {
*
* as absolute file paths with forward slashes.
*/
export function normalizeFilename(filename: string, config: AstroConfig) {
export function normalizeFilename(filename: string, root: URL) {
if (filename.startsWith('/@fs')) {
filename = filename.slice('/@fs'.length);
} else if (filename.startsWith('/') && !ancestor(filename, config.root.pathname)) {
const url = new URL('.' + filename, config.root);
} else if (filename.startsWith('/') && !ancestor(filename, fileURLToPath(root))) {
const url = new URL('.' + filename, root);
filename = viteID(url);
}
return removeLeadingForwardSlashWindows(filename);
Expand Down