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 JsonMissingBlock Check #651

Merged
merged 5 commits into from
Dec 10, 2024
Merged

Add JsonMissingBlock Check #651

merged 5 commits into from
Dec 10, 2024

Conversation

navdeep5
Copy link
Contributor

@navdeep5 navdeep5 commented Dec 4, 2024

What are you adding in this PR?

Addresses https://github.com/Shopify/shopify/issues/552694

This PR introduces enhanced validation checks for JSON template files, specifically ensuring that:

  1. Block types exist as valid files in the blocks/ dir
  2. Block types are allowed in their respective section or block at the root-level

image
image
image

What's next? Any followup issues?

With this being a new check, we will want to update https://github.com/Shopify/shopify-dev so users can understand their errors. Developer Docs changes: https://github.com/Shopify/shopify-dev/pull/51342

What did you learn?

I have never worked or come across JSON template files before so this was fun to learn about.

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
      • If applicable, I've updated the theme-app-extension.yml config
  • I included a minor bump changeset
  • My feature is backward compatible
  • I included a patch bump changeset

@navdeep5 navdeep5 force-pushed the navdeep-json-missing-block branch from 05b1074 to d7436b4 Compare December 5, 2024 16:57
@navdeep5 navdeep5 changed the title Inital dump Add JsonMissingBlock Check Dec 5, 2024
@navdeep5 navdeep5 marked this pull request as ready for review December 5, 2024 19:56
Copy link
Contributor

@miazbikowski miazbikowski left a comment

Choose a reason for hiding this comment

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

I noticed that if I just open my json file and put whatever non-existent type as a block type, it doesn't complain. It will only complain if that non-existent block type is also in the parent's schema as an accepted block. Is that by design? I was wonder if we should verify the json block types for existence independently of the schema, but I figured maybe there's a reason for this behaviour.

Copy link
Contributor

@miazbikowski miazbikowski left a comment

Choose a reason for hiding this comment

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

We have that issue with onCodePathEnd checks not running when the editor opens, but rather on refocusing on the file tab, but that's not strictly related to this check. Otherwise, I tophatted and code lgtm.

@miazbikowski
Copy link
Contributor

Bit of a captain obvious but I'll write it out just in case, but I didn't check that the util functions don't duplicate logic that's also used elsewhere and that we'd benefit from extracting out of that check. I'm thinking you just kept that's specific to that check in there.

Copy link
Contributor

@albchu albchu left a comment

Choose a reason for hiding this comment

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

Change LGTM and tophats as expected! Great work Nav! I added one minor nit but feel free to ingore 😏

@navdeep5 navdeep5 merged commit 15995a7 into main Dec 10, 2024
6 checks passed
@navdeep5 navdeep5 deleted the navdeep-json-missing-block branch December 10, 2024 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants