add block param nodes #1552
Merged
add block param nodes #1552
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 tasks
NullVoxPopuli
approved these changes
Jan 25, 2024
| let [Foo] = ast.body; | ||
| if (assertNodeType(Foo, 'BlockStatement')) { | ||
| let [ab, cd, efg] = guardArray({ blockParamNodes: Foo.program.blockParamNodes }, { min: 3 }); | ||
| locEqual(ab, 1, 11, 1, 13); |
There was a problem hiding this comment.
can the value / name be asserted as well?
There was a problem hiding this comment.
i think this should be done separately. i will add it in parser-node-test
NullVoxPopuli
requested changes
Jan 25, 2024
patricklx
force-pushed
the
param-node
branch
2 times, most recently
from
1ed5c46
to
a88263f
Compare
| @@ -293,6 +295,16 @@ export interface HashPair extends BaseNode { | |||
| value: Expression; | |||
| } | |||
|
|
|||
| /** | |||
| * a param inside the pipes of elements or mustache blocks, | |||
There was a problem hiding this comment.
🙌 perfect! thank you!
| let [Foo] = ast.body; | ||
| if (assertNodeType(Foo, 'ElementNode')) { | ||
| let [ab, cd, efg] = guardArray({ blockParamNodes: Foo.blockParamNodes }, { min: 3 }); | ||
| locEqual(ab, 1, 8, 1, 12); |
There was a problem hiding this comment.
since the signature for locEqual is
function locEqual(
node: AST.Node | null | undefined,
startLine: number,
startColumn: number,
endLine: number,
endColumn: number,
message = JSON.stringify(node)
) {should endColumn be 2 characters more than startColumn? right now it sees like.. 4?
locEqual(ab, 1, 8, 1, 12); // 12 - 8 = 4, but the node is 'ab', 2
locEqual(cd, 1, 12, 1, 15); // 15 - 12 = 3, but the node is 'cd', 2
locEqual(efg, 1, 15, 1, 19); // 19 - 15 = 4, but the node is 'efg', 3There was a problem hiding this comment.
oh! thanks for noticing. since those are initially parsed as attributes by hbs parser the location contains the pipe as well... need to fix it. not sure why it also contains the whitespace, might be a bug in hbs parser
't=<Foo as |ab cd efg|></Foo>'
t.slice(8,12)
'|ab '
t.slice(12,15)
'cd '
t.slice(15,19)
'efg|'
NullVoxPopuli
approved these changes
Jan 26, 2024
chancancode
added a commit
that referenced
this pull request
Feb 25, 2024
The original commit has a few issues: - Type safety - Added a new AST node, which is unnecessary since we already have `VarHead`, but is also a breaking change and is kind of breakage we have been careful to avoid in this area thus far - The parsing is robust against a few classes of edge cases This rework the same feature (on the `Block`/`BlockStatement` side) and adds more test coverage for the feature. Also reanmed the field to `block.params`.
chancancode
added a commit
that referenced
this pull request
Feb 25, 2024
The original commit has a few issues: - Type safety - Added a new AST node, which is unnecessary since we already have `VarHead`, but is also a breaking change and is kind of breakage we have been careful to avoid in this area thus far - The parsing is robust against a few classes of edge cases This rework the same feature (on the `Block`/`BlockStatement` side) and adds more test coverage for the feature. Also reanmed the field to `block.params`.
chancancode
added a commit
that referenced
this pull request
Feb 25, 2024
The original commit has a few issues: - Type safety - Added a new AST node, which is unnecessary since we already have `VarHead`, but is also a breaking change and is kind of breakage we have been careful to avoid in this area thus far - The parsing is robust against a few classes of edge cases This rework the same feature (on the `Block`/`BlockStatement` side) and adds more test coverage for the feature. Also reanmed the field to `block.params`.
chancancode
added a commit
that referenced
this pull request
Feb 25, 2024
The original commit has a few issues: - Type safety - Added a new AST node, which is unnecessary since we already have `VarHead`, but is also a breaking change and is kind of breakage we have been careful to avoid in this area thus far - The parsing is robust against a few classes of edge cases This rework the same feature (on the `Block`/`BlockStatement` side) and adds more test coverage for the feature. Also reanmed the field to `block.params`.
chancancode
added a commit
that referenced
this pull request
Feb 26, 2024
The original commit has a few issues: - Type safety - Added a new AST node, which is unnecessary since we already have `VarHead`, but is also a breaking change and is kind of breakage we have been careful to avoid in this area thus far - The parsing is robust against a few classes of edge cases This rework the same feature (on the `Block`/`BlockStatement` side) and adds more test coverage for the feature. Also renamed the field to `block.params`.
chancancode
added a commit
that referenced
this pull request
Feb 27, 2024
The previous/existing "parsing" code tries to piggy back on letting the HTML tokenizer/parse parse out the block params syntax into attribute nodes and tries to recover the information after the fact This is fundamentally broken and suffers from some wild edge cases like: ``` <div as="" |foo="bar|>...</div> // => <div as |foo|>...</div> ``` This is especially problematic in light of wanting to retain the source spans of the block params nodes. This commit rework the block params parsing and integrate it into the normal parsing pass in the appropiate time.
chancancode
added a commit
that referenced
this pull request
Feb 27, 2024
The previous/existing "parsing" code tries to piggy back on letting the HTML tokenizer/parser parse out the block params syntax into attribute nodes and tries to recover the information after the fact This is fundamentally broken and suffers from some wild edge cases like: ``` <div as="" |foo="bar|>...</div> // => <div as |foo|>...</div> ``` This is especially problematic in light of wanting to retain the source spans of the block params nodes. This commit rework the block params parsing and integrate it into the normal parsing pass in the appropiate time.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
part of #1490
made it backwards compatible