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

[LiquidDoc][Theme Check] Type validations on parameters passed to {% render %} tag #792

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

Conversation

jamesmengo
Copy link
Contributor

@jamesmengo jamesmengo commented Feb 14, 2025

What are you adding in this PR?

Closes https://github.com/Shopify/developer-tools-team/issues/536

https://share.descript.com/view/i7Q9QsBv3tJ

Validates the type of a parameter provided to a snippet against the type outline in the doc header (if present)

Rules / Exceptions

  • type in the header is case insensitive (i.e. {boOlEAn} is valid)
  • booleans will accept anything, because everything in Liquid is truthy / falsy - source
  • If a VariableLookup is provided, we don't validate the type. We just accept the value: {% render 'product-card', num_param: num_variable %} <- we would need to evaluate num_variable

Before you deploy

  • This PR includes a new checks or changes the configuration of a check
    • I included a minor bump changeset
    • It's in the allChecks array in src/checks/index.ts
    • I ran yarn build and committed the updated configuration files
      • [s] If applicable, I've updated the theme-app-extension.yml config
    • I've made a PR to update the shopify.dev theme check docs if applicable (link PR here). <- todo in batch

Copy link
Contributor Author

jamesmengo commented Feb 14, 2025

invalidValues: [
{ value: '123', expectedType: SupportedParamTypes.Number },
{ value: 'true', expectedType: SupportedParamTypes.Boolean },
{ value: 'product', expectedType: SupportedParamTypes.Object },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

todo: A VariableLookup should probably just default to valid, since we can only validate this with the runtime value

],
},
{
type: 'boolean',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@charlespwd Could you give me a sanity check here?

Truthy / Falsy handling for booleans

Given the documentation on types, I may need to revisit how we validate Boolean to accept ANYTHING, since everything is truthy or falsy - link. This means that anything can be passed down as a boolean param - is that correct?


Complex Objects

I looked at types of LiquidNamedArgument.value.type, and tried to map those to a "supported type". Any complex types, such as product are just object (I took inspiration from existing comments in Dawn on this)

@jamesmengo jamesmengo changed the title Implement a new Theme Check to validate parameters provided to snippets - Check for missing required parameters - Validate parameter types - Report unknown parameters [LiquidDoc][Theme Check] Type validations WIP Feb 14, 2025
@jamesmengo jamesmengo force-pushed the jm/base_ld_check branch 2 times, most recently from 9a2f51d to 06b96c5 Compare February 15, 2025 19:47
@jamesmengo jamesmengo changed the base branch from jm/base_ld_check to graphite-base/792 February 18, 2025 16:01
@jamesmengo jamesmengo changed the base branch from graphite-base/792 to main February 18, 2025 16:02
@jamesmengo jamesmengo force-pushed the jm/param_type_check branch 4 times, most recently from 75ab8e0 to f69929d Compare February 25, 2025 00:48
…ender snippets

- Reports type mismatches
- Suggests autofixes (replace with default or remove value)
- Skips type checking for variable lookups
    - We would need to evaluate this dynamically, so we should accept this until we can do so
- Skips type checking for unknown parameters
- Skips type checking for unknown types
@jamesmengo jamesmengo removed the request for review from charlespwd February 25, 2025 01:39
@jamesmengo jamesmengo changed the title [LiquidDoc][Theme Check] Type validations WIP [LiquidDoc][Theme Check] Type validations on parameters passed to {% render %} tag Feb 25, 2025
@jamesmengo jamesmengo marked this pull request as ready for review February 25, 2025 01:39
@jamesmengo jamesmengo requested a review from a team as a code owner February 25, 2025 01:39
@jamesmengo jamesmengo added the #gsd:44310 LiquidDoc label Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
#gsd:44310 LiquidDoc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant