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

For linting, we'd need to be able to emit a full AST, including nodes for the template #39

Open
NullVoxPopuli opened this issue Nov 29, 2023 · 7 comments · May be fixed by #40
Open

For linting, we'd need to be able to emit a full AST, including nodes for the template #39

NullVoxPopuli opened this issue Nov 29, 2023 · 7 comments · May be fixed by #40

Comments

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Nov 29, 2023

In order to get the index / replacement / splice management out of eslint-plugin-ember or other tooling, additional APIs may be needed -- or a unified AST may need to be provided -- or more tooling so folks don't have to manage the template removing / insertion again.

eslint-plugin-ember

  • runs @glimmer/syntax over the template and re-stitches together AST nodes
    • enables type-aware linting
    • enables one lint tool (eslint) instead of two (template-lint)
      • enables fixers to be able to be implemented with the whole context of a module

prettier-plugin-ember-template-tag & ember-template-lint

  • current strategy:
    • extract templates
    • format JS
    • format templates with offsets
    • place templates back in placeholder spots

Motivation

  • index splicing is really easy to get wrong in javascript.
  • any <template> manipulation really belongs with the tool that already is supporting content-tag, because it can more easily assure that parsing and serialization is correct.

Ideas:

format

makes the prettier plugin implementation easier.
the intermediary format is never seen.

import { format } from 'content-tag';

let outputGjsString = format(gjsString, (contentTagContent, positionInfo /* for reporting */) => {
  // no-op
  return contentTagContent;

  // or, 
  return prettier(contentTagContent, { parser: 'glimmer'});
});

parseAst + serializeAst

This would produce a JSON AST for the whole JS + <template> contents, and would require @glimmer/syntax for now, but I've volunteered myself to learn Rust and have a go at a rust-based glimmer template parser.

import { parseAst, serializeAst } from 'content-tag';
import { parse /* or whatever this was called */ } from '@glimmer/syntax';

// no <template> or intermediary format seen in here
let ast = parseAst(gjsString, { glimmerParse: parse });

let outputGjs = serializeAst(ast);

cc @patricklx @wagenet, @gitKrystan

@patricklx patricklx linked a pull request Nov 29, 2023 that will close this issue
2 tasks
@patricklx
Copy link

I need to look at eslint plugin again. I'm not sure this will work there.
As we need to pass the source to the eslint typescript parser, because it needs to do some preprocessing on the ast to collect identifiers, references, scopes. Not sure if we can just pass the ast, and also not sure if the ast would be compatible

@patricklx
Copy link

I think it would be best to provide functions which:

  • provide a transformed code which does not change offsets and is parsable by other tools.
  • receives an ast with visitor keys mapping, finds the template nodes that are currently represented as e.g object expression or function expression and replaces that node with a template node.

I think that would be the best approach and would support a wider range of tools

@NullVoxPopuli
Copy link
Contributor Author

some clarifying questions:

provide a transformed code which does not change offsets and is parsable by other tools.

Why is not changing offsets important?
Is this so you don't have to iterate over all the nodes correcting all the offsets due to the number of lines changes after the transform?

finds the template nodes that are currently represented

Why exactly do we need template nodes parsed?

technically, with the content-tag proposal, any language could be used within a content-tag, not just glimmer

@patricklx
Copy link

patricklx commented Nov 29, 2023

Is this so you don't have to iterate over all the nodes correcting all the offsets due to the number of lines changes after the transform?

yes, especially for linting

Why exactly do we need template nodes parsed?

i just meant the ast node. it doesnt need to parse the glimmer content. just have 1 node with e.g. type=GlimmerTemplate. no child nodes.

The other tools do not know about it, so template tag must be replaces with some other known JS code. the ast node of code can be found later and replaced. (like its done in eslint plugin now)

@ef4
Copy link
Collaborator

ef4 commented Nov 29, 2023

Yeah, two things here, and I think we should do both:

  1. Exposing the full AST as @patricklx does in print ast #40. I think that's a good feature for content-tag and it would support any tooling where you're able to completely replace the parser and pass an AST forward.

  2. Making the process() API take an optional callback that controls what template tags get replaced with. This would cover all the cases where people are doing splicing today. Glint, for example, wants to pass the results to typescript and it would want its callback to actually invoke the handlebars parser and convert the whole thing into their behind-the-scenes TS representation of the template.

    This callback would receive the template tag's contents as a string and return a JS string that would replace the whole template tag. It should also have access to something like we offer in https://github.com/emberjs/babel-plugin-ember-template-compilation/blob/038cabde1709c49751c3224df66e6b4adfc4b60d/src/js-utils.ts#L111, so that it has a declarative way to get access to imported identifiers.

    The default value of the callback that implements today's hard-coded behavior would look something like:

    function transform(src: string, type: "expression" | "class-member", utils) {
      let t = utils.bindImport("@ember/template-compiler", "template");
      if (type === "expression") {
        return `${t}("${utils.escapeStringLiteral(
          src
        )}", { eval() { return eval(arguments[0]) } }`
      } else {
        return `static { 
          ${t}("${utils.escapeStringLiteral(src)}", {
            eval() { return eval(arguments[0]) },
            component: this,
          }) 
        }`;
      }
    }

@patricklx
Copy link

That sounds good.
So it would not do ast transform? Only direct replacement in strings? Because that's what we need for eslint plugin.
To keep all whitespace, semi etc the same

@NullVoxPopuli
Copy link
Contributor Author

Yeah, two things here, and I think we should do both:

This would probably also unblock reporting parsing errors on incorrect lines in: typed-ember/glint#655

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 a pull request may close this issue.

3 participants