Validate Astro frontmatter JS/TS on compiler error #2115

FredKSchott · 2021-12-03T19:52:48Z

Changes

Before passing a component to the compiler, use esbuild to validate the frontmatter against JS/TS syntax.
This will prevent a class of bug where bad front-matter syntax can break the compiled output in very obscure ways (for example, an unclosed parenthesis).
Once the compiler can handle this better, we can remove this.

Before

---
import Layout from '../layouts/MainLayout.astro';
{
---

Unclear error output.

After

Clear error output, with the correct bug location. "end of file" is still a little odd wording (it should say "end of frontmatter", which we can fix by re-writing this error message if that makes sense) but this is already a nice improvement over broken output, compiler panics, etc.

Testing

Still TODO

Docs

Not needed

changeset-bot · 2021-12-03T19:52:50Z

🦋 Changeset detected

Latest commit: 7bf85a3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
astro	Patch

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

netlify · 2021-12-03T19:54:14Z

✔️ Deploy Preview for astro-www ready!

🔨 Explore the source changes: b487414

🔍 Inspect the deploy log: https://app.netlify.com/sites/astro-www/deploys/61aa759143dd430008646dc7

😎 Browse the preview: https://deploy-preview-2115--astro-www.netlify.app

netlify · 2021-12-03T19:54:56Z

✔️ Deploy Preview for astro-docs-2 ready!

🔨 Explore the source changes: b487414

🔍 Inspect the deploy log: https://app.netlify.com/sites/astro-docs-2/deploys/61aa75913ff0f2000ad00f03

😎 Browse the preview: https://deploy-preview-2115--astro-docs-2.netlify.app/guides/styling

FredKSchott · 2021-12-03T20:10:12Z

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

@@ -88,6 +97,7 @@ export default function astro({ config, devServer }: AstroPluginOptions): vite.P
 if (cssTransformError) throw cssTransformError;

 // Compile `.ts` to `.js`
+ console.log(tsResult.code);


doh, this should be removed. Thanks lint-bot!

drwpow

Pretty clever! This makes a lot of sense to me. And good error output

matthewp · 2021-12-03T20:55:59Z

Can we do this in the catch instead? That way the cost is only occurred on code that has an error anyways and not for every compile that we do.

FredKSchott · 2021-12-03T20:59:28Z

@matthewp clever! yes, that makes a ton of sense.

I'll make that change, clean the PR up, and some tests, and submit for re-review.

natemoo-re

On board with the concept, have reservations about this apporach.

Since the Astro compiler already does a lexical scan, what if it exposed a utility to output the raw frontmatter? We could then validate it separately from the rest of the generated file.

natemoo-re · 2021-12-03T20:59:31Z

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

+ // The compiler doesn't currently validate the frontmatter, so invalid JS/TS can cause issues
+ // in the final output. This prevents bad JS from becoming a compiler panic or otherwise
+ // breaking the final output.
+ const scannedFrontmatter = /^\-\-\-(.*)^\-\-\-/ms.exec(source);


This RegExp approach would be too brittle IMO, this should use a lexical scan.

@natemoo-re if we can't validate it inside of the compiler, what if the compiler exported a function like validateFrontmatter or even validateJsExpression so we could run it on template fragments as well? I little nervous about this being async though, if called on every template expression.

Could this be a short-term fix, and then a longer-term fix would be to add this kind of checking into the Go compiler itself later on down the road? We'll probably want the same for valid CSS...

If we follow @matthewp’s “only try on catch” strategy then this should only improve the experience. IMO the lexical scan won’t be as important because on catch we know for sure this is invalid JS. Then this would only be for error display and nothing more. So if by some happenstance that RegEx failed, it would only return a weird error message on edge cases (would would have been weird anyway, so same experience), but in most cases would vastly improve the UX.

I'm on board with that reasoning and this being the short-term fix. No objections to moving forward with Matthew's suggested approach.

FredKSchott · 2021-12-03T21:00:25Z

I'll also rewrite "unexpected end of file" to "unexpected end of frontmatter" if there are no objections.

drwpow · 2021-12-04T01:01:44Z

IMO this doesn’t need a test, but if you do add one, adding a dev test in errors.test.js would be the easiest solution because there are very similar tests there.

netlify · 2021-12-07T00:12:21Z

✔️ Deploy Preview for astro-www ready!

🔨 Explore the source changes: 71e47a1

🔍 Inspect the deploy log: https://app.netlify.com/sites/astro-www/deploys/61aea68024aac5000741f704

😎 Browse the preview: https://deploy-preview-2115--astro-www.netlify.app

netlify · 2021-12-07T00:12:51Z

✔️ Deploy Preview for astro-docs-2 ready!

🔨 Explore the source changes: 71e47a1

🔍 Inspect the deploy log: https://app.netlify.com/sites/astro-docs-2/deploys/61aea680804b4e000794f470

😎 Browse the preview: https://deploy-preview-2115--astro-docs-2.netlify.app

netlify · 2021-12-07T00:14:00Z

✔️ Deploy Preview for astro-www ready!

🔨 Explore the source changes: f7da6db

🔍 Inspect the deploy log: https://app.netlify.com/sites/astro-www/deploys/61aea7569ac477000796bf5b

😎 Browse the preview: https://deploy-preview-2115--astro-www.netlify.app

netlify · 2021-12-07T00:15:11Z

✔️ Deploy Preview for astro-docs-2 ready!

🔨 Explore the source changes: 869adf8

🔍 Inspect the deploy log: https://app.netlify.com/sites/astro-docs-2/deploys/61aea6c084aebb000795e8f5

😎 Browse the preview: https://deploy-preview-2115--astro-docs-2.netlify.app

FredKSchott · 2021-12-07T00:15:07Z

packages/astro/test/errors.test.js

 });

 it('runtime error', async () => {
- if (isMacOS) return;
-
+ // if (isMacOS) return;


note: i can run these locally, so I figured I'd uncomment to see what happens.

If they pass, I'll remove entirely.
If they fail, I'll uncomment.

update: tests are passing in CI so I'll remove completely
update: removed

netlify · 2021-12-07T00:17:52Z

✔️ Deploy Preview for astro-docs-2 ready!

🔨 Explore the source changes: f7da6db

🔍 Inspect the deploy log: https://app.netlify.com/sites/astro-docs-2/deploys/61aea756f9f9730008bd5751

😎 Browse the preview: https://deploy-preview-2115--astro-docs-2.netlify.app

matthewp

looks great

FredKSchott · 2021-12-07T16:20:09Z

works great in build too:

$ astro build
Error: Transform failed with 1 error:
/Users/fks/Code/astro/docs/src/pages/index.astro:4:0: error: Unexpected end of frontmatter

* validate the astro component frontmatter ahead of compilation * add test, update existing tests * chore(lint): Prettier fix * Update index.ts * remove macos skip lines, no longer needed Co-authored-by: GitHub Action <[email protected]>

github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Dec 3, 2021

FredKSchott force-pushed the validate-frontmatter branch from b6cc26a to dd285a0 Compare December 3, 2021 20:01

FredKSchott commented Dec 3, 2021

View reviewed changes

drwpow approved these changes Dec 3, 2021

View reviewed changes

natemoo-re reviewed Dec 3, 2021

View reviewed changes

FredKSchott self-assigned this Dec 6, 2021

FredKSchott added 2 commits December 6, 2021 15:39

validate the astro component frontmatter ahead of compilation

ac4853f

add test, update existing tests

71e47a1

FredKSchott force-pushed the validate-frontmatter branch from 43bb887 to 71e47a1 Compare December 7, 2021 00:10

github-actions bot added the test label Dec 7, 2021

chore(lint): Prettier fix

869adf8

FredKSchott changed the title ~~Validate Astro frontmatter JS/TS ahead of compilation~~ Validate Astro frontmatter JS/TS on compiler error Dec 7, 2021

Update index.ts

f7da6db

FredKSchott commented Dec 7, 2021

View reviewed changes

remove macos skip lines, no longer needed

7bf85a3

matthewp approved these changes Dec 7, 2021

View reviewed changes

natemoo-re approved these changes Dec 7, 2021

View reviewed changes

FredKSchott merged commit 0ef682c into main Dec 7, 2021

FredKSchott deleted the validate-frontmatter branch December 7, 2021 16:18

github-actions bot mentioned this pull request Dec 7, 2021

[ci] release #2149

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate Astro frontmatter JS/TS on compiler error #2115

Validate Astro frontmatter JS/TS on compiler error #2115

FredKSchott commented Dec 3, 2021 •

edited

Loading

changeset-bot bot commented Dec 3, 2021 •

edited

Loading

netlify bot commented Dec 3, 2021

netlify bot commented Dec 3, 2021

FredKSchott Dec 3, 2021

drwpow left a comment

matthewp commented Dec 3, 2021

FredKSchott commented Dec 3, 2021

natemoo-re left a comment

natemoo-re Dec 3, 2021

FredKSchott Dec 3, 2021

drwpow Dec 4, 2021 •

edited

Loading

natemoo-re Dec 5, 2021

FredKSchott commented Dec 3, 2021

drwpow commented Dec 4, 2021

netlify bot commented Dec 7, 2021

netlify bot commented Dec 7, 2021

netlify bot commented Dec 7, 2021 •

edited

Loading

netlify bot commented Dec 7, 2021

FredKSchott Dec 7, 2021

FredKSchott Dec 7, 2021 •

edited

Loading

netlify bot commented Dec 7, 2021

matthewp left a comment

FredKSchott commented Dec 7, 2021

Validate Astro frontmatter JS/TS on compiler error #2115

Validate Astro frontmatter JS/TS on compiler error #2115

Conversation

FredKSchott commented Dec 3, 2021 • edited Loading

Changes

Before

After

Testing

Docs

changeset-bot bot commented Dec 3, 2021 • edited Loading

🦋 Changeset detected

netlify bot commented Dec 3, 2021

netlify bot commented Dec 3, 2021

FredKSchott Dec 3, 2021

Choose a reason for hiding this comment

drwpow left a comment

Choose a reason for hiding this comment

matthewp commented Dec 3, 2021

FredKSchott commented Dec 3, 2021

natemoo-re left a comment

Choose a reason for hiding this comment

natemoo-re Dec 3, 2021

Choose a reason for hiding this comment

FredKSchott Dec 3, 2021

Choose a reason for hiding this comment

drwpow Dec 4, 2021 • edited Loading

Choose a reason for hiding this comment

natemoo-re Dec 5, 2021

Choose a reason for hiding this comment

FredKSchott commented Dec 3, 2021

drwpow commented Dec 4, 2021

netlify bot commented Dec 7, 2021

netlify bot commented Dec 7, 2021

netlify bot commented Dec 7, 2021 • edited Loading

netlify bot commented Dec 7, 2021

FredKSchott Dec 7, 2021

Choose a reason for hiding this comment

FredKSchott Dec 7, 2021 • edited Loading

Choose a reason for hiding this comment

netlify bot commented Dec 7, 2021

matthewp left a comment

Choose a reason for hiding this comment

FredKSchott commented Dec 7, 2021

FredKSchott commented Dec 3, 2021 •

edited

Loading

changeset-bot bot commented Dec 3, 2021 •

edited

Loading

drwpow Dec 4, 2021 •

edited

Loading

netlify bot commented Dec 7, 2021 •

edited

Loading

FredKSchott Dec 7, 2021 •

edited

Loading