-
Notifications
You must be signed in to change notification settings - Fork 30
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 "On section rename" handling #707
Conversation
When a `sections/*.liquid` file is renamed, we'll update automatically update references in the following locations: - `templates/*.json` files that were using the old name - `sections/*.json` files that were using the old name - Static section calls of the form `{% section 'old-name' %}` Fixes #546
9c93229
to
1deaf0f
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.
🎩 ✅
- rename liquid section file -> update template json
- rename liquid section file -> update section json
- rename liquid section file -> update
{% section %}
usage
few questions/minor comments
packages/theme-language-server-common/src/renamed/handlers/SectionRenameHandler.ts
Show resolved
Hide resolved
packages/theme-language-server-common/src/renamed/handlers/SectionRenameHandler.ts
Outdated
Show resolved
Hide resolved
// Note the type assertion to the LHS of the expression. | ||
// The type assertions above are enough for this to be true. | ||
// But I'm making the explicit annotation here to make it clear. | ||
const typeNode: LiquidString = node.markup; | ||
if (typeNode.value !== oldSectionName) return; |
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.
Do you need to explicitly have it as LiquidString
? When i removed it, it still showed up as LiquidString
when i hovered over typeNode
.
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.
Yep. IIRC I felt like future proofing against node.markup being possibly something else here. It didn't feel obvious at the time that the type assertions made the typeNode a LiquidString from the code being used.
packages/theme-language-server-common/src/renamed/handlers/SectionRenameHandler.spec.ts
Outdated
Show resolved
Hide resolved
- `templates/*.json` files that referenced the old file name | ||
- `sections/*.json` files that referenced the old file name | ||
- Static section calls formatted as `{% section 'old-name' %}` |
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.
What about config/settings_data.json
?
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.
You can have a section in settings_data.json?!
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.
Didn't see anything about that in the docs 🤔? What am I missing?
https://shopify.dev/docs/storefronts/themes/architecture/sections#render-a-section
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.
Hmm what is this then? https://github.com/Shopify/dawn/blob/main/config/settings_data.json#L173-L185
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.
:o
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.
I have no idea haha. Doesn't seem documented 🤔
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.
OK given this would require TS interfaces and probably a revisit of block renames as well, I'll log this as a different issue and merge this as is to prevent from ballooning the PR
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.
sounds good 👍
What are you adding in this PR?
When files within the
sections/*.liquid
directory are renamed, we will update all references to these files in their previous locations. This includes:templates/*.json
files that referenced the old file namesections/*.json
files that referenced the old file name{% section 'old-name' %}
NOTE: This is very similar to block renames but because types are different and things are only so slightly different, I'm not sure it's worth DRY'ing it up as it would make complicated code even more complicated
Fixes #546
on.section.rename.mp4
Before you deploy
changeset