Closed patricklx closed 7 months ago
@gitKrystan i think this works. But you would need to remove most of the ambiguous cases...
Thanks. I will look into this ASAP. Might not be until next week though.
btw, i didn't remove the tests for the ambiguous parts. Wanted to wait on content-tag.
Wanted to wait on content-tag.
how do you mean? is there an issue / failing test for that project?
Can you re-run pnpm install
? Seems like the lock file is out of date.
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) => {}
I'm adding failing tests for those to content-tag now :tada:
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?
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. 🙏
Looks like there are a few different types of test failure:
Cause unknown. This seems to be the majority of the failures.
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.
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
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| }
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.
Thanks again for working on this!
mmm, still 600 failing tests in ci... I'll check again tomorrow
vitest was somehow auto updating all tests snapshots... this will need more work
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
I can probably revert the semi change by changing the ast type to function expression for the template. I think.
After reverting all the snapshot changes, it looks like a lot of the failures are unexpected export default
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
@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
now all green
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
content-tag does not like vite build.
Content-tag in browsers won't work until this is merged: https://github.com/embroider-build/content-tag/pull/18
Demo of usage: https://github.com/NullVoxPopuli/limber/blob/main/packages/ember-repl/addon/src/browser/gjs.ts#L14
cc @ef4
@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
@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.
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.
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
@gitKrystan squashed commits, its ready now
I did the update for eslint, and though that it might work for this as well.
design:
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.Template
Ast node, which holds the template content.estree
Prettier printer, which formats the valid JS above.Template
AST node described above.Template
AST node with the results from above, wrapped in<template>
others:
</template> as TemplateOnlyComponent<Signature>
will be left to prettiers default handling