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 keyword arguments for Liquid shortcodes #3444

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

nex3
Copy link

@nex3 nex3 commented Sep 15, 2024

This is an adaptation of #1733 to use the built-in Liquid argument parser that was added in #2679. Keyword arguments are passed as an object after all the positional arguments.

@nex3 nex3 requested a review from zachleat as a code owner September 15, 2024 10:11
@zachleat
Copy link
Member

Hmm, I’m seeing some test suite failures here from some missing variables in Liquid.js

Copy link
Member

@zachleat zachleat left a comment

Choose a reason for hiding this comment

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

This is looking great but has a few small issues!

I think the big question I have is in the test suite tests—I would imagine these tests would need to be gated behind eleventyConfig.setLiquidParameterParsing("builtin"); similar to what shipped in #2679 (comment)

src/Engines/Liquid.js Outdated Show resolved Hide resolved
src/Engines/Liquid.js Show resolved Hide resolved
let value = tokenizer.readHash() ?? tokenizer.readValue();
// readHash() treats unmarked identifiers as hash keys with undefined
// values, but we want to parse them as positional arguments instead.
return value?.kind === 64 && value.value === undefined ? value.name : value;
Copy link
Member

Choose a reason for hiding this comment

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

I’m seeing some errors here, from LiquidJS’s types: value and name do not exist on type HashToken | ValueToken | RangeToken

Copy link
Author

Choose a reason for hiding this comment

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

That's weird, I would expect Liquid to be typed so that the value.kind check type-infers value to be a HashToken. I've made that explicit.

nex3 and others added 3 commits September 19, 2024 18:54
This is an adaptation of 11ty#1733 to use the built-in Liquid argument
parser that was added in 11ty#2679.

Co-Authored-By: Dylan Awalt-Conley <[email protected]>
Copy link
Author

@nex3 nex3 left a comment

Choose a reason for hiding this comment

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

The tests keep timing out locally, so I'm not 100% confident they're passing now. But I did fix all the known issues!

let value = tokenizer.readHash() ?? tokenizer.readValue();
// readHash() treats unmarked identifiers as hash keys with undefined
// values, but we want to parse them as positional arguments instead.
return value?.kind === 64 && value.value === undefined ? value.name : value;
Copy link
Author

Choose a reason for hiding this comment

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

That's weird, I would expect Liquid to be typed so that the value.kind check type-infers value to be a HashToken. I've made that explicit.

@nex3
Copy link
Author

nex3 commented Sep 21, 2024

I'm clearly missing something about how to correctly set the parameter parsing...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants