Refactor Astro plugin and compile flow #5506
Conversation
🦋 Changeset detectedLatest commit: be75a66 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 |
| // For CSS / hoisted scripts, the main Astro module should already be cached | ||
| const filename = normalizeFilename(parsedId.filename, config); | ||
| const compileResult = getCachedCompileResult(config, filename); | ||
| if (!compileResult) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
I found this.resolve and normalizeFilename seems to guard the same thing conceptually, so I opt for normalizeFilename as it's done by other plugins too.
| 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, { |
There was a problem hiding this comment.
Swapping our use of esbuild.transform to transformWithEsbuild so we use Vite's esbuild instance, and get the benefits of respecting the tsconfig.
I'm planning to refactor this more for the JSX plugin to when I backport vitejs/vite#11120 to Vite 3.
There was a problem hiding this comment.
Oh interesting, how was our esbuild not respecting it?
There was a problem hiding this comment.
Vite has this code that passes the resolved tsconfig for that file during transform, as esbuild doesn't handle that by default. It should make the compiled output a bit more accurate, but in most case not noticable.
| // Also, catches invalid JS/TS in the compiled output before returning. | ||
| esbuildResult = await transformWithEsbuild(transformResult.code, rawId, { | ||
| loader: 'ts', | ||
| target: 'esnext', |
There was a problem hiding this comment.
Use esnext as we don't need down-levelling (same behaviour as esbuild.transform and how Vite works currently).
| } | ||
| } | ||
|
|
||
| export async function cachedCompilation(props: CompileProps): Promise<CompileResult> { |
There was a problem hiding this comment.
I like that the caching part is split into a separate module now!
| // For CSS / hoisted scripts, the main Astro module should already be cached | ||
| const filename = normalizeFilename(parsedId.filename, config); | ||
| const compileResult = getCachedCompileResult(config, filename); | ||
| if (!compileResult) { |
There was a problem hiding this comment.
I'm not sure this is right. At least looking at the old code we used to do await cachedCompilation(compileProps); for styles. Although thinking about it now, there shouldn't be a time where styles are loaded before we have compiled 🤔
There was a problem hiding this comment.
I noticed that for styles too but we always guarded this part with a:
let source = getCachedSource(config, raw.id);
if (!source) {
return null;
}So technically we're expecting the main module to be compiled before we process the styles, otherwise we bail.
There was a problem hiding this comment.
I would verify that all of the tests pass with this.
|
I love how much smaller the module has gotten! Good refactor, not sure why the tests are failing though. |
|
Windows has blessed me with a green check 😌 (though I had to rerun once but I'll take it) Turns out I made some changes to |
Changes
vite-plugin-astroto split of core logic out ofindex.tsso they can be tested./@fsusage when compiling (usesresolve.dedupeto achieve this)It's easier to review the changes by checking each commit individually.
Testing
All existing tests should pass.
Docs
N/A. refactors.