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

TypeScript AST #40

Merged
merged 4 commits into from
Dec 3, 2021
Merged

TypeScript AST #40

merged 4 commits into from
Dec 3, 2021

Conversation

sh0rez
Copy link
Member

@sh0rez sh0rez commented Nov 28, 2021

Introduces a (printing only) TypeScript AST and refactors the generator to use it:

  • ts/ast: structs with String() methods for printing what they represent
  • ts: higher level package, things like Union for generating a union type. Not heavily used yet.
  • loosely inspired by the go/ast internals

Sets up a struct-based TypeScript ast, loosely inspired by the go/ast package.

Notable:
- no position information, as we only do rendering, not parsing
- rendering is handled using String() functions, not a separate printer package
Makes the generator "use" the ast by returning a []ast.Node instead of writing into a
bytes.Buffer.
Those ast.Nodes however are of type ast.Raw, so the ast itself is not
really used yet.
@sh0rez sh0rez self-assigned this Nov 28, 2021
Copy link
Contributor

@sdboyer sdboyer left a comment

Choose a reason for hiding this comment

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

O.M.G. This is just wonderful. I'm so delighted to see this!

Tests are passing, and as long as that's the case, i'm happy to merge whenever. What more do you feel ought to be done before merging? And, for anything we defer, i'd love to see brief follow-up issues posted, so that we don't lose track of the TODOs.

return []byte("\n" + file.String()), nil
}

func GenerateAST(val cue.Value, c Config) (*ts.File, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing i like about having GenerateAST() with a return type of ts.File is the opportunity it affords the caller to more easily mess around with the final output - e.g., how Grafana has to, for now at least, inject its own import statement(s).

To that end - unless i missed it? - i'd love to see AST types for representing TS imports.

@@ -21,8 +21,7 @@ type {{.name}} = {{ Join .tokens " | "}};
{{- if .default }}

{{if .export}}export {{end -}}
const default{{.name}}: {{.name}} = {{.default}};{{end}}
`)
const default{{.name}}: {{.name}} = {{.default}};{{end}}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

i hate templates, lolsob

return fmt.Sprintf("%s %s: %s = %s;", tok, v.Name, v.Type, v.Value)
}

type Type interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

My brain is stuttering over all the use of Type. Can we use Kind instead, particularly for consistency with the attribute names?

@sdboyer
Copy link
Contributor

sdboyer commented Nov 30, 2021

Oh also, just to note - there's another half of the story here, perhaps harder than this, where we create some intermediate structures to represent our analysis of the CUE inputs rather than just working with raw cue.Value. It's not exactly AST, and it's higher-level information than what's in CUE's internal adt package. But i think it's key to making all the logic we have here more maintainable - it allows us to separate the inference of what some property is - given the particular rules we have about @cuetsy attributes, references, etc. - from what we want to do with that property.

But that can, and should, proceed orthogonally to this. Just making the note :)

@sh0rez sh0rez marked this pull request as ready for review December 3, 2021 14:41
@sdboyer sdboyer merged commit 03fb9c4 into main Dec 3, 2021
@sdboyer sdboyer deleted the sh0rez/ast branch December 3, 2021 14:41
@sdboyer
Copy link
Contributor

sdboyer commented Dec 8, 2021

@sh0rez nudge to create those follow-up issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants