add Preprocessor.parse #22
Conversation
|
Thanks for working on this. I think the other PR I landed today ensures that all the spans you need are already available in the AST. So that might simplify some of this. |
Co-authored-by: zvkemp <[email protected]> Co-authored-by: chancancode <[email protected]>
zvkemp
force-pushed
the
19-parse-metadata
branch
from
740a7ac
to
66bfb43
Compare
| let result = self | ||
| .core | ||
| .parse(&src) | ||
| .map_err(|_err| self.process(src, filename).unwrap_err())?; |
There was a problem hiding this comment.
This calls process just to get the error, eventually it can be extracted out into some common method, but this works for now
| .parse(&src) | ||
| .map_err(|_err| self.process(src, filename).unwrap_err())?; | ||
| let serialized = serde_json::to_string(&result) | ||
| .map_err(|err| js_error(format!("Unexpected serialization error; please open an issue with the following debug info: {err:#?}").into()))?; |
There was a problem hiding this comment.
This should never happen, so if it does it is a bug
| fn from(value: &Span) -> Self { | ||
| Range { | ||
| start: value.lo.0 as usize - 1, | ||
| end: value.hi.0 as usize - 1, |
There was a problem hiding this comment.
It appears that SWC spans are 1-based for some reason, while our existing API expects them to be 0-based. Or that there is a bug in the swc fork. Either way, this fixes the issue and satisfies the existing expectations. If it is indeed a bug upstream and we fix it, we can potentially remove the -1 here but we shouldn't change the expectations in the tests.
There was a problem hiding this comment.
Yes, they are weirdly 1-based and they reserve 0 to mean the dummy span.
| start: 16, | ||
| end: 27 | ||
| } | ||
| }, |
There was a problem hiding this comment.
The API is slightly different from the existing one, in that we normalized all the ranges to { start: number, end: number }, whereas the existing API uses [start: number, end: number] for some seemingly randomly
|
|
||
| it("preceded by a slash character", function() { | ||
| let p = new Preprocessor(); | ||
| // What is this testing? |
There was a problem hiding this comment.
the ember-templace-imports parser historically broke every time you had a / before <template>
| ); | ||
| }).to.throw(`Parse Error at path/to/my/component.gjs:2:15: 2:15`); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
do you think it's worth adding a test for a multi-line <template> tag?
and then maybe one with:
<template>
y
<template>x</template>
z
</template>This should parse out to 1 set of ranges, because the inner <template> is just HTML
There was a problem hiding this comment.
That is actually not legal. The first </template> closes the content tag. Because content-tag is deliberately agnostic to the interior language, it doesn't know anything about delimiter pairing.
| fn from(value: &Span) -> Self { | ||
| Range { | ||
| start: value.lo.0 as usize - 1, | ||
| end: value.hi.0 as usize - 1, |
There was a problem hiding this comment.
Yes, they are weirdly 1-based and they reserve 0 to mean the dummy span.
src/lib.rs
Outdated
| end_range: closing.span.into(), | ||
| start_range: opening.span.into(), | ||
| tag_name: "template".to_owned(), | ||
| kind: "template-tag".to_owned(), |
There was a problem hiding this comment.
kind: "template-tag" is not doing anything useful here, since it's the only kind that's supported and that will likely ever be supported. In the earlier implementation it existed to distinguish between this and the hbs-backticks alternative proposal.
Instead, we should change this to have the value "expression" or "class-member" because that is highly-salient information that is already present in the AST. There were significant hacks required to work around not having that information in places like the eslint plugin, which is a big part of what motivated content-tag in the first place.
There was a problem hiding this comment.
The AST doesn't currently differentiate it, so it's outside of the scope of the current PR, but we should eventually have a declaration kind too. The current AST just treat it as a top-level free-floating expression which isn't quite right. However, I want to refactor the AST to have a common ContentTag node, which ContentTagExpression and ContentTagMember can wrap, similar to how Function is the shared type for FunctionExpression and Getter, etc. That should make it easier to share code between the AST visitors.
There was a problem hiding this comment.
As for the use case for declaration: for example it would make it easier for the downstream tools to enforce that there can only be one per module, which isn't necessarily the case for all use cases of content tag, but it is for the <template> specification.
|
Published in 1.1.0 |
fixes #19. We still need to review test assertions and do a fair bit of cleanup