Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add basic {% doc %} tag parsing support #621

Merged
merged 1 commit into from
Dec 4, 2024
Merged

Add basic {% doc %} tag parsing support #621

merged 1 commit into from
Dec 4, 2024

Conversation

jamesmengo
Copy link
Contributor

@jamesmengo jamesmengo commented Nov 27, 2024

What are you adding in this PR?

https://github.com/Shopify/develop-advanced-edits/issues/441

Adds support for Liquid doc tags ({% doc %}) in the LiquidHTML parser.
This is the basic implementation which we will iterate on.

What's next? Any followup issues?

  • I would like to explore implementing autocomplete for the doc tag to have a working example of full iteration of adding a change
  • Next would be the param tag.

What did you learn?

  • I learned about how the parser tokenizes our input strings
  • I learned about how the grammar produces match results and those are combined with mappings to transform the tokenized inputs into structured data.

Before you deploy

  • I included a minor bump changeset <- waiting until we built on top of this to add a changeset
  • My feature is backward compatible

@jamesmengo jamesmengo force-pushed the jm/doc_tag branch 2 times, most recently from bdfdee9 to 9f70770 Compare November 27, 2024 22:28
@jamesmengo jamesmengo force-pushed the jm/doc_tag branch 3 times, most recently from 31923d9 to c2768ee Compare December 3, 2024 00:37
Copy link
Contributor Author

jamesmengo commented Dec 3, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

@jamesmengo jamesmengo changed the title Add liquidDoc tag to liquidHTMLParser Add basic {% doc %} tag parsing support Dec 3, 2024
@jamesmengo jamesmengo force-pushed the jm/doc_tag branch 2 times, most recently from 8e2f374 to d1954f6 Compare December 3, 2024 01:34
@jamesmengo jamesmengo self-assigned this Dec 3, 2024
@jamesmengo jamesmengo marked this pull request as ready for review December 3, 2024 01:36
@jamesmengo jamesmengo mentioned this pull request Dec 3, 2024
8 tasks
Comment on lines +629 to +652
liquidDoc: {
type: ConcreteNodeTypes.LiquidRawTag,
name: 'doc',
body: (tokens: Node[]) => tokens[1].sourceString,
children: (tokens: Node[]) => {
const contentNode = tokens[1];
return toLiquidDocAST(
source,
contentNode.sourceString,
offset + contentNode.source.startIdx,
);
},
whitespaceStart: (tokens: Node[]) => tokens[0].children[1].sourceString,
whitespaceEnd: (tokens: Node[]) => tokens[0].children[7].sourceString,
delimiterWhitespaceStart: (tokens: Node[]) => tokens[2].children[1].sourceString,
delimiterWhitespaceEnd: (tokens: Node[]) => tokens[2].children[7].sourceString,
locStart,
locEnd,
source,
blockStartLocStart: (tokens: Node[]) => tokens[0].source.startIdx,
blockStartLocEnd: (tokens: Node[]) => tokens[0].source.endIdx,
blockEndLocStart: (tokens: Node[]) => tokens[2].source.startIdx,
blockEndLocEnd: (tokens: Node[]) => tokens[2].source.endIdx,
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs an override for {% liquid %}. If not in this PR, it should be created as an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*/
function toLiquidDocAST(source: string, matchingSource: string, offset: number) {
// When we switch parser, our locStart and locEnd functions must account
// for the offset of the {% liquid %} markup
Copy link
Contributor

@charlespwd charlespwd Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my bad copy paste, this should say doc :)

Copy link
Contributor

@charlespwd charlespwd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. We could add a changeset here, but—since there are no public API changes—I think we're OK.

Add stage-2 ast tests for doc tags
@jamesmengo jamesmengo added the #gsd:43130 Liquid DX label Dec 4, 2024
@jamesmengo jamesmengo merged commit 606304f into main Dec 4, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
#gsd:43130 Liquid DX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants