Refactor sortByLoc to use @glimmer/syntax' sortByLoc function #662
Conversation
Previously a node was cloned with `JSON.parse(JSON.stringify(node))`. This approach needs to be sophisticated because the node.loc property has been changed from a plain JSON object to a `SourceSpan` instance in in v0.66.0 of @glimmer/syntax. (See PR 1170 in https://github.com/glimmerjs/glimmer-vm if you want to take a look at the changes in @glimmer/syntax.) node.loc in v0.65.4 has the following interface: ``` export interface SourceLocation { source?: Option<string>; start: Position; end: Position; } ``` reference: https://github.com/glimmerjs/glimmer-vm/blob/v0.65.4/packages/%40glimmer/syntax/lib/types/nodes.ts#L36-L40 in v0.80.0, node.loc is an instance of SourceSpan, which is defined here: https://github.com/glimmerjs/glimmer-vm/blob/v0.80.0/packages/%40glimmer/syntax/lib/source/loc/span.ts#L106-L309
Added in @glimmer/syntax v0.76.0: glimmerjs/glimmer-vm#1266
dcyriller
changed the title
| @@ -1,5 +1,6 @@ | |||
| import { preprocess, builders, print as _print, traverse, ASTv1 as AST } from '@glimmer/syntax'; | |||
| import { getLines, sortByLoc, sourceForLoc } from './utils'; | |||
| import { klona } from 'klona/full'; | |||
There was a problem hiding this comment.
How is this related to the changes to move to use sortByLoc from @glimmer/syntax?
Also, FWIW, the sortByLoc that landed in @glimmer/syntax was originally extracted from us 😄
There was a problem hiding this comment.
Also, wouldn't we want to use klona/json? Or do we need other structures now?
There was a problem hiding this comment.
The sortByLoc function expects the node.loc property to be an instance of the SourceSpan class. But JSON.parse(JSON.stringify()) and klona/json can only clone JSON data types. When cloned with JSON.parse(JSON.stringify()), the data structure is simplified (to a SourceLocation?). So sortByLoc fails on startPosition being undefined.
There was a problem hiding this comment.
I am closing the PR as I don't have a good solution for now.
Description
Starting from v0.76.0, a
sortByLocfunction is exposed in @glimmer/syntax. An idea is to use it here in template-recast to replace a very similar function (see glimmerjs/glimmer-vm#1266 (comment)).This PR is a possible implementation. But IMO an other implementation should be done. Indeed, this PR would add another dependency and would cause a slow down.
See commit messages for more details.
Links
ElementNodeglimmerjs/glimmer-vm#1266 (comment)