Upgrade @glimmer/syntax to v0.82.0 #627
Conversation
|
Have you seen #401? It bumps glimmer syntax to 0.79.0. If we merge it, would it ease this upgrade? |
|
@dcyriller nope, missed it, thank you for reference! I will try to merge it |
|
@dcyriller @rwjblue looks like it's working |
lifeart
force-pushed
the
attempt-to-get-latest-glimmer-syntax
branch
from
21bdb1f to
4e03d48
Compare
dcyriller
changed the title
dcyriller
force-pushed
the
attempt-to-get-latest-glimmer-syntax
branch
2 times, most recently
from
d117dd2 to
c9c9559
Compare
There was a problem hiding this comment.
So first thank you for working on this @lifeart!
Sorry, it takes me time to review, I wanted to look at the changes in glimmer-vm, in particular I need to understand what ASTv1 is vs. ASTv2.
I rebased your commit and pushed another one. In this last commit I change the imports from import { ASTv1 } from ... to import { ASTv1 as AST }.
dcyriller
force-pushed
the
attempt-to-get-latest-glimmer-syntax
branch
from
76e18b7 to
f9de578
Compare
|
I pushed a couple other commits @lifeart. It looks ready to me. Do you mind taking another look? |
Only one comment, changes itself looks good! |
dcyriller
force-pushed
the
attempt-to-get-latest-glimmer-syntax
branch
from
f9de578 to
dcf5118
Compare
dcyriller
changed the title
This is a workaround to a change in builders.element
Here is how a synthetic node used to be defined in glimmer/syntax
v0.65.4:
```
export const SYNTHETIC: AST.SourceLocation = {
source: '(synthetic)',
start: { line: 1, column: 0 },
end: { line: 1, column: 0 },
};
```
(source https://github.com/glimmerjs/glimmer-vm/blob/v0.65.4/packages/@glimmer/syntax/lib/builders.ts#L509-L513)
With the isSynthetic function, we want to know if a node is:
- synthetic (meaning created with a builder)
- has no location set -> if a location has been set, it will be used to
position the node
see https://github.com/glimmerjs/glimmer-vm/blob/v0.65.4/packages/@glimmer/syntax/lib/builders.ts#L509-L513 ``` export const SYNTHETIC: AST.SourceLocation = { source: '(synthetic)', start: { line: 1, column: 0 }, end: { line: 1, column: 0 }, }; ```
dcyriller
force-pushed
the
attempt-to-get-latest-glimmer-syntax
branch
from
aa1636d to
9012b37
Compare
|
So... I think we've come to the most minimal changes to upgrade @glimmer/syntax from v0.65.0 to v0.82.0 I've rewritten the git history one last time, hoppping for a good documentation of the changes we made. I've run the ember-template-lint test suite against this PR - it will make some tests fail (see #401 (comment) for more information). I've run some manual perf benchmarks to confirm this upgrade doesn't have a negative impact.
Thanks a lot @lifeart! |
looks like few errors from "fixers" for older ast syntax versions, and few from "glimmer-syntax" itself (like paddings for attribues)