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

Handle compiler breaking change #5803

Merged
merged 10 commits into from
Jan 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/dull-rabbits-relax.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'astro': patch
---

Upgrade compiler and handle breaking changes
2 changes: 0 additions & 2 deletions .npmrc
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,5 @@ public-hoist-pattern[]=github-slugger
# Vite's esbuild optimizer has trouble optimizing `@astrojs/lit/client-shim.js`
# which imports this dependency.
public-hoist-pattern[]=@webcomponents/template-shadowroot
# On Windows, `svelte-preprocess` can't import `svelte/compiler`. Might be a pnpm bug.
public-hoist-pattern[]=svelte
Comment on lines -13 to -14
Copy link
Member Author

Choose a reason for hiding this comment

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

We don't have the svelte-preprocess dependency anymore, so this can be removed.

# There's a lit dependency duplication somewhere causing multiple Lit versions error.
public-hoist-pattern[]=*lit*
3 changes: 3 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@
"react",
"react-dom",
"@types/react"
],
"allowAny": [
"astro"
Comment on lines +70 to +71
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 is added to suppress the peer dep warning of astro-embed, since it relied on Astro 1.0, but we want to test in 2.0.

]
},
"patchedDependencies": {
Expand Down
2 changes: 1 addition & 1 deletion packages/astro/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@
"test:e2e:match": "playwright test -g"
},
"dependencies": {
"@astrojs/compiler": "^0.31.4",
"@astrojs/compiler": "^0.32.0",
"@astrojs/language-server": "^0.28.3",
"@astrojs/markdown-remark": "^2.0.0-beta.1",
"@astrojs/telemetry": "^1.0.1",
Expand Down
22 changes: 15 additions & 7 deletions packages/astro/src/core/compile/compile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import type { ResolvedConfig } from 'vite';
import type { AstroConfig } from '../../@types/astro';

import { transform } from '@astrojs/compiler';
import { fileURLToPath } from 'url';
import { normalizePath } from 'vite';
import { AggregateError, AstroError, CompilerError } from '../errors/errors.js';
import { AstroErrorData } from '../errors/index.js';
import { resolvePath } from '../util.js';
Expand Down Expand Up @@ -35,15 +37,11 @@ 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: filename,
pathname: filename,
projectRoot: astroConfig.root.toString(),
site: astroConfig.site?.toString(),
sourcefile: filename,
filename,
normalizedFilename: normalizeFilename(filename, astroConfig.root),
sourcemap: 'both',
internalURL: 'astro/server/index.js',
// TODO: baseline flag
experimentalStaticExtraction: true,
astroGlobalArgs: JSON.stringify(astroConfig.site),
preprocessStyle: createStylePreprocessor({
filename,
viteConfig,
Expand Down Expand Up @@ -111,3 +109,13 @@ function handleCompileResultErrors(result: TransformResult, cssTransformErrors:
}
}
}

function normalizeFilename(filename: string, root: URL) {
const normalizedFilename = normalizePath(filename);
const normalizedRoot = normalizePath(fileURLToPath(root));
if (normalizedFilename.startsWith(normalizedRoot)) {
return normalizedFilename.slice(normalizedRoot.length - 1);
} else {
return normalizedFilename;
}
}
9 changes: 2 additions & 7 deletions packages/astro/src/runtime/server/astro-global.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,8 @@ function createAstroGlobFn() {
}

// This is used to create the top-level Astro global; the one that you can use
// Inside of getStaticPaths.
// TODO: remove `_filePathname` and `_projectRootStr` from the compiler
export function createAstro(
_filePathname: string,
site: string | undefined,
_projectRootStr: string
): AstroGlobalPartial {
// inside of getStaticPaths. See the `astroGlobalArgs` option for parameter type.
export function createAstro(site: string | undefined): AstroGlobalPartial {
return {
site: site ? new URL(site) : undefined,
generator: `Astro v${ASTRO_VERSION}`,
Expand Down
57 changes: 42 additions & 15 deletions packages/astro/src/vite-plugin-astro/hmr.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
import { fileURLToPath } from 'node:url';
import type { HmrContext, ModuleNode } from 'vite';
import type { AstroConfig } from '../@types/astro';
import { cachedCompilation, invalidateCompilation, isCached } from '../core/compile/index.js';
import {
cachedCompilation,
CompileResult,
invalidateCompilation,
isCached,
} from '../core/compile/index.js';
import type { LogOptions } from '../core/logger/core.js';
import { info } from '../core/logger/core.js';
import * as msg from '../core/messages.js';
Expand All @@ -17,32 +22,24 @@ export interface HandleHotUpdateOptions {
config: AstroConfig;
logging: LogOptions;
compile: () => ReturnType<typeof cachedCompilation>;
source: string;
}

export async function handleHotUpdate(
ctx: HmrContext,
{ config, logging, compile }: HandleHotUpdateOptions
{ config, logging, compile, source }: HandleHotUpdateOptions
) {
let isStyleOnlyChange = false;
if (ctx.file.endsWith('.astro') && isCached(config, ctx.file)) {
// Get the compiled result from the cache
const oldResult = await compile();
// But we also need a fresh, uncached result to compare it to
// Skip HMR if source isn't changed
if (oldResult.source === source) return [];
// Invalidate to get fresh, uncached result to compare it to
invalidateCompilation(config, ctx.file);
const newResult = await compile();
// If the hashes are identical, we assume only styles have changed
if (oldResult.scope === newResult.scope) {
if (isStyleOnlyChanged(oldResult, newResult)) {
isStyleOnlyChange = true;
// All styles are the same, we can skip an HMR update
const styles = new Set(newResult.css);
for (const style of oldResult.css) {
if (styles.has(style)) {
styles.delete(style);
}
}
if (styles.size === 0) {
return [];
}
Comment on lines -30 to -45
Copy link
Member Author

Choose a reason for hiding this comment

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

Since scope hash now derives from normalizedFilename only, I created a isStyleOnlyChanged function to handle it. It's a bit tedious than before, but I think leaving the HMR logic out of the compiler could be nice for now.

I also remove the styles.size === 0 handling. The logic isn't quite right given a scenario where:

  1. oldResult has two <style>s
  2. User removes one <style>
  3. newResult now has one <style>
  4. This logic makes styles.size === 0 and skips HMR.
  5. BUG: The removed style's side effect isn't removed as no HMR or reload is done.

It seems that this does handle skipping HMR if no source code change, which I handled on line 37 instead.

}
} else {
invalidateCompilation(config, ctx.file);
Expand Down Expand Up @@ -134,3 +131,33 @@ export async function handleHotUpdate(

return mods;
}

function isStyleOnlyChanged(oldResult: CompileResult, newResult: CompileResult) {
return (
normalizeCode(oldResult.code) === normalizeCode(newResult.code) &&
!isArrayEqual(oldResult.css, newResult.css)
);
}

const astroStyleImportRE = /import\s*"[^"]+astro&type=style[^"]+";/g;
const sourceMappingUrlRE = /\/\/# sourceMappingURL=[^ ]+$/gm;

/**
* Remove style-related code and sourcemap from the final astro output so they
* can be compared between non-style code
*/
function normalizeCode(code: string) {
return code.replace(astroStyleImportRE, '').replace(sourceMappingUrlRE, '').trim();
}

function isArrayEqual(a: any[], b: any[]) {
if (a.length !== b.length) {
return false;
}
for (let i = 0; i < a.length; i++) {
if (a[i] !== b[i]) {
return false;
}
}
return true;
}
1 change: 1 addition & 0 deletions packages/astro/src/vite-plugin-astro/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ export default function astro({ settings, logging }: AstroPluginOptions): vite.P
config,
logging,
compile,
source: compileProps.source,
});
},
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"version": "0.0.0",
"private": true,
"dependencies": {
"astro": "^1.0.0",
"astro": "workspace:*",
Copy link
Member Author

Choose a reason for hiding this comment

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

The 1.0.0 was used to bypass the peer dep warning, but I found this test to use the old 1.0 code instead, breaking the compiler upgrade as both 1.0 and 2.0 code exist at the same time.

"astro-embed": "^0.1.1"
}
}
7 changes: 4 additions & 3 deletions packages/astro/test/units/compile/invalid-css.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,17 @@ import { resolveConfig } from 'vite';
import { expect } from 'chai';
import { cachedCompilation } from '../../../dist/core/compile/index.js';
import { AggregateError } from '../../../dist/core/errors/index.js';
import { pathToFileURL } from 'url';

describe('astro/src/core/compile', () => {
describe('Invalid CSS', () => {
it('throws an aggregate error with the errors', async () => {
let error;
try {
let r = await cachedCompilation({
astroConfig: /** @type {any} */ ({
root: '/',
}),
astroConfig: {
root: pathToFileURL('/'),
},
viteConfig: await resolveConfig({ configFile: false }, 'serve'),
filename: '/src/pages/index.astro',
source: `
Expand Down
3 changes: 2 additions & 1 deletion packages/astro/test/units/vite-plugin-astro/compile.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { expect } from 'chai';
import { resolveConfig } from 'vite';
import { cachedFullCompilation } from '../../../dist/vite-plugin-astro/compile.js';
import { init, parse } from 'es-module-lexer';
import { pathToFileURL } from 'url';

const viteConfig = await resolveConfig({ configFile: false }, 'serve');

Expand All @@ -12,7 +13,7 @@ const viteConfig = await resolveConfig({ configFile: false }, 'serve');
async function compile(source, id) {
return await cachedFullCompilation({
compileProps: {
astroConfig: { root: '/', base: '/' },
astroConfig: { root: pathToFileURL('/'), base: '/' },
viteConfig,
filename: id,
source,
Expand Down
Loading