content tag

[patricklx]

I did the update for eslint, and though that it might work for this as well.

design:

1. Run Preprocessor.parse from content-tag to get template locations. manually convert gjs files with <template> tags into valid JS, which doesn't change location. The <template> tag gets converted into empty objects.
2. convert empty object AST node to a custom Template Ast node, which holds the template content.
3. Run the estree Prettier printer, which formats the valid JS above.
4. Grab template contents from Template AST node described above.
5. Run the hbs Prettier printer against the template contents.
6. Replace the Template AST node with the results from above, wrapped in <template>

others:
</template> as TemplateOnlyComponent<Signature> will be left to prettiers default handling

[patricklx]

@gitKrystan i think this works. But you would need to remove most of the ambiguous cases...

[gitKrystan]

Thanks. I will look into this ASAP. Might not be until next week though.

[patricklx]

btw, i didn't remove the tests for the ambiguous parts. Wanted to wait on content-tag.

[NullVoxPopuli]

Wanted to wait on content-tag.

how do you mean? is there an issue / failing test for that project?

[gitKrystan]

Can you re-run pnpm install? Seems like the lock file is out of date.

[patricklx]

embroider-build/content-tag#37

[gitKrystan]

The purpose of the ambiguous cases in the tests is that we don't introduce syntax errors when prettifying.

e.g.

<template>Hello World</template>
(oops) => {}

We should be able to detect this case and print:

<template>Hello World</template>;
(oops) => {}

[NullVoxPopuli]

I'm adding failing tests for those to content-tag now 🎉

[patricklx]

The purpose of the ambiguous cases in the tests is that we don't introduce syntax errors when prettifying.

e.g.

<template>Hello World</template>
(oops) => {}

We should be able to detect this case and print:

<template>Hello World</template>;
(oops) => {}

It would be converted to something like

export default template()
() => null

How would it happen that you introduce syntax errors? Although it works with typescript.
It cannot be parsed already by babel
Maybe it cannot happen anymore now?

[gitKrystan]

I have lots of thoughts, but no idea which ones are relevant. Please push the lockfile changes so I can run the tests and see what you are talking about. 🙏

[gitKrystan]

Looks like there are a few different types of test failure:

Missing visitor keys for 'Template'.

Cause unknown. This seems to be the majority of the failures.

Ambiguous cases that are now syntax errors.

With the previous ember-template-imports parsing, these were not a syntax error.

Example of a more realistic case.

const ListItem = <template><li>My Item</li></template> // no semi

<template>
  <ul>
    <ListItem />
    <ListItem />
    <ListItem />
  </ul>
</template>

If content-tag team says these are now syntax errors, we can continue to error as we do now and remove the relevant tests. (See below first, though.)

If content-tag team says these are not syntax errors, the issue needs to be fixed in the content-tag repo.

Change of behavior in other ambiguous cases.

These are still not considered syntax errors in the content-tag world, but how we are handling it seems to have changed. Will have to diff the snapshot output to determine how, but it's probably some combo of new-lines and semi-colons.

Here's an example of where the behavior change appears to be introducing a syntax error where there was none. The user added a semi-colon to prevent the syntax error and this plugin removed it.

[test:run]  FAIL  tests/unit-tests/ambiguous/arrow-parens-avoid.test.ts > ambiguous > config > arrowParens: "avoid" > (oh, no) => {} > with semi, with newline > it formats ../cases/gjs/default-export.gjs
[test:run] Error: Snapshot `ambiguous > config > arrowParens: "avoid" > (oh, no) => {} > with semi, with newline > it formats ../cases/gjs/default-export.gjs 1` mismatched
[test:run] 
[test:run] - Expected
[test:run] + Received
[test:run] 
[test:run]   "<template>
[test:run]     Explicit default export module top level component. Explicit default export
[test:run]     module top level component. Explicit default export module top level
[test:run]     component. Explicit default export module top level component. Explicit
[test:run]     default export module top level component.
[test:run] - </template>;
[test:run] + </template>
[test:run]   (oh, no) => {};
[test:run]   "
[test:run] 
[test:run]  ❯ behavesLikeFormattedAmbiguousCase tests/helpers/ambiguous.ts:95:20
[test:run]      93|   try {
[test:run]      94|     const result = await format(code, formatOptions);
[test:run]      95|     expect(result).toMatchSnapshot();
[test:run]        |                    ^
[test:run]      96|   } catch (error: unknown) {
[test:run]      97|     // Some of the ambiguous cases are Syntax Errors when parsed
[test:run]  ❯ tests/helpers/ambiguous.ts:64:13

Change of behavior with config templateExportDefault: true

Cause unknown. Maybe this case just isn't handled yet. With templateExportDefault: true, we should include export default for the relevant template export.

[test:run]  FAIL  tests/unit-tests/config/template-export-default.test.ts > config > templateExportDefault: true > it formats ../cases/gjs/default-export.gjs
[test:run] Error: Snapshot `config > templateExportDefault: true > it formats ../cases/gjs/default-export.gjs 1` mismatched
[test:run] 
[test:run] - Expected
[test:run] + Received
[test:run] 
[test:run] - "export default <template>
[test:run] + "<template>
[test:run]     Explicit default export module top level component. Explicit default export
[test:run]     module top level component. Explicit default export module top level
[test:run]     component. Explicit default export module top level component. Explicit
[test:run]     default export module top level component.
[test:run]   </template>
[test:run]   "
[test:run] 
[test:run]  ❯ tests/helpers/make-suite.ts:59:20
[test:run]      57|     const code = testCase.code.replaceAll(AMBIGUOUS_PLACEHOLDER, '');
[test:run]      58|     const result = await format(code, config.options);
[test:run]      59|     expect(result).toMatchSnapshot();
[test:run]        |                    ^
[test:run]      60|   });
[test:run]      61| }

[patricklx]

I think i missed something when pushing...
Locally i didn't have Missing visitor keys for 'Template'. and had like ~300 failing tests
Will check tomorrow.
Basically there were only syntax errors.

[gitKrystan]

Thanks again for working on this!

[patricklx]

mmm, still 600 failing tests in ci... I'll check again tomorrow

[patricklx]

vitest was somehow auto updating all tests snapshots... this will need more work

[patricklx]

okay, now down to 360.
There are some changes to semi handling.

Also, some export default where added... Not sure if they should be there .
I don't think they should as default is false for it.
But somehow the option.exportDefault is true

[patricklx]

I can probably revert the semi change by changing the ast type to function expression for the template. I think.

[NullVoxPopuli]

After reverting all the snapshot changes, it looks like a lot of the failures are unexpected export default

[patricklx]

Yes. There is also 1 bug at
https://github.com/gitKrystan/prettier-plugin-ember-template-tag/pull/162/files#diff-ed9c6e22579caeb649360bf79dac90d5222c8de99f5c31968cf53ff3825175eeL379

But note that i only add the export default if it's enabled in the options:
https://github.com/gitKrystan/prettier-plugin-ember-template-tag/pull/162/files#diff-fdd9b7cec43f9e7c596cd66d5e1656a3fc2ad2c0ae98cc7a165273a560a022e1R105

[patricklx]

@gitKrystan are you okay with this changes to semi handling?
note that in the the end < template > will be converted to a function call, not a function declaration.
shall we change the ast node to CallExpression instead?
and what shall we do about the syntax errors. delete the tests causing it? I just changed the catch clause to handle the parse errors from content-tag

it looks like you expected this to happen?

In this case, some of the ambiguous expressions may result in a syntax error, in which case our Prettier plugin would error instead of formatting, similar to how Prettier already handles plain JavaScript syntax errors. Other ambiguous expressions may have their "ambiguity" resolved within the parser, allowing them to be printed as the recommendations show above.

see link

[patricklx]

now all green

[patricklx]

nope, example is failing. content-tag does not like vite build.
can we just tsc build to dist, or do you want to have this work in browser env as well?

edit: just set content-tag as external, now works. but browser env will not

[NullVoxPopuli]

content-tag does not like vite build.

Content-tag in browsers won't work until this is merged: embroider-build/content-tag#18

Demo of usage: https://github.com/NullVoxPopuli/limber/blob/main/packages/ember-repl/addon/src/browser/gjs.ts#L14

cc @ef4

[patricklx]

@gitKrystan I think this is ready.

I wonder if the ambiguous handling is still relevant with content-tag?
its using a custom parser to transform the code, so syntax errors will cause an error here as well.
So now the ambiguous handling only has impact at runtime. maybe it should be the responsibility of eslint or glint to warn the user of constructs like
<template></template> + x
<template></template>['as']

edit: I realized now that its not ready... some ambiguous constructs actually can cause content-tag to fail parsing.
So custom semi handling is required,
I was trying to emulate it with AST nodes, but that does not seem possible

[patricklx]

@gitKrystan i was able to minimize the changes to semi handling.
what do you think?
i also thought of another way to check if a semi is required or not. by printing the next node with semi: false, look at the result and then fixup the previous builders.doc result.
i will cleanup the code more later.

[gitKrystan]

Looks like we picked up an accidental export default here.

[patricklx]

its because i'm leaving as TemplateOnlyComponent<Signature> to prettiers default handling.
Since it now remains an expression, we cannot safely remove or add default exports to it.
are you okay with this?

[gitKrystan]

Seems fine. Let's see if anyone complains.

[gitKrystan]

Thanks again for all this work!

Re: the cases causing syntax errors. Let's keep them for now since they get swallowed anyway. As we've discovered, what counts as a syntax error might change as the implementation of content-tag evolves.

Re: ambiguous cases, we want to follow Prettier's current patterns as discussed here. We can/should update that analysis if necessary since content-tag doesn't produce an array like ember-template-imports did. Check out this example.

[patricklx]

ambiguous cases:
i think the only difference is when there are multiple templates in succession. then no semi is needed anymore.

<template></template>
<template></template>

its either wrapped in a static {} for class properties or exported default. or will end up with a function call, which also works without semi

[patricklx]

@gitKrystan squashed commits, its ready now