-
Notifications
You must be signed in to change notification settings - Fork 24
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
Check for Valid Block Targeting #561
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really great start here. Thanks Nav!
packages/theme-check-common/src/checks/valid-block-target/index.spec.ts
Outdated
Show resolved
Hide resolved
packages/theme-check-common/src/checks/valid-block-target/index.spec.ts
Outdated
Show resolved
Hide resolved
packages/theme-check-common/src/checks/valid-block-target/index.spec.ts
Outdated
Show resolved
Hide resolved
packages/theme-check-common/src/checks/valid-block-target/index.spec.ts
Outdated
Show resolved
Hide resolved
packages/theme-check-common/src/checks/valid-block-target/index.spec.ts
Outdated
Show resolved
Hide resolved
packages/theme-check-common/src/checks/valid-block-target/index.spec.ts
Outdated
Show resolved
Hide resolved
packages/theme-check-common/src/checks/valid-block-target/index.spec.ts
Outdated
Show resolved
Hide resolved
packages/theme-check-common/src/checks/valid-block-target/index.spec.ts
Outdated
Show resolved
Hide resolved
packages/theme-check-common/src/checks/valid-block-target/index.spec.ts
Outdated
Show resolved
Hide resolved
packages/theme-check-common/src/checks/valid-block-target/index.ts
Outdated
Show resolved
Hide resolved
8e60308
to
3d3d201
Compare
packages/theme-check-common/src/checks/valid-block-target/block-utils.ts
Outdated
Show resolved
Hide resolved
packages/theme-check-common/src/checks/valid-block-target/block-utils.ts
Outdated
Show resolved
Hide resolved
packages/theme-check-common/src/checks/valid-block-target/block-utils.ts
Outdated
Show resolved
Hide resolved
packages/theme-check-common/src/checks/valid-block-target/index.ts
Outdated
Show resolved
Hide resolved
packages/theme-check-common/src/checks/valid-block-target/index.ts
Outdated
Show resolved
Hide resolved
packages/theme-check-common/src/checks/valid-block-target/block-utils.ts
Outdated
Show resolved
Hide resolved
packages/theme-check-common/src/checks/valid-block-target/index.ts
Outdated
Show resolved
Hide resolved
packages/theme-check-common/src/checks/valid-block-target/index.ts
Outdated
Show resolved
Hide resolved
8e17417
to
07cb7ac
Compare
63907e6
to
e65cb0c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes and tophat lgtm. The presets as hash stuff also works like magic.
May you also get eyes from Mia and CP on this before we merge?
What are you adding in this PR?
Resolves #554 and #555
This PR introduces enhanced validation checks for theme blocks, combining two main functionalities:
Soft Validation of Nested Blocks
blocks
array OR@theme
block type must be declared at the root level (exceptions: private blocks)_
) must always be explicitly declared at the root level.File Existence Check
We are combining these checks into one as they are both responsible for ensuring we have "valid block targets" and it also decreases the number of I/Os required.
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/50568
What did you learn?
This is my first time contributing to this repo so there was a lot to learn in general. However, something that I think will be useful for following checks is being able to traverse the JSON AST of the liquid schema in a controlled and robust manner.
Before you deploy
changeset
allChecks
array insrc/checks/index.ts
yarn build
and committed the updated configuration filestheme-app-extension.yml
configchangeset
changeset