Add block rename handler (replace old->new everywhere!) #655
Conversation
charlespwd
force-pushed
the
feature/block-rename-handling
branch
from
9b21030 to
15cf74e
Compare
|
Going to try and get the jsonc PR #656 merged as soon as we can here before assessing what kind of assistance we can apply to this PR before CP returns from holiday break. |
Saves you some work to get position info
charlespwd
force-pushed
the
feature/block-rename-handling
branch
2 times, most recently
from
ae65e1f to
f40aba4
Compare
|
I'm picking this back up with Navdeep's first stab at the tests. Couldn't figure out what his email is for the correct co-author to appear on the commit. |
Co-authored-by: Navdeep Singh <[email protected]> Whenever a block gets renamed, the following will now happen: 1. References in files with a {% schema %} will be changed 2. References in template files will be changed 3. References in section groups will be changed 4. References in {% content_for "block", type: "oldName" %} will be changed
charlespwd
force-pushed
the
feature/block-rename-handling
branch
from
f40aba4 to
51fe22e
Compare
packages/theme-language-server-common/src/renamed/handlers/AssetRenameHandler.ts
Outdated
Show resolved
Hide resolved
packages/theme-language-server-common/src/renamed/handlers/BlockRenameHandler.ts
Show resolved
Hide resolved
charlespwd
force-pushed
the
feature/block-rename-handling
branch
from
634e83f to
c99d769
Compare
There was a problem hiding this comment.
1 weird hiccup during tophat 🎩
- When i renamed
blocks/texttoblocks/poetry, i noticedblocks/accordion's schema still had it as"text"- I assumed it was because it had some syntax errors. I fixed those errors, and i ran "rename" again
- When i did the rename, it only changed
accordion's reference, but not the other files
Without changing accordion (everything works as expected)
Screen.Recording.2025-01-08.at.10.03.44.AM.mov
After changing accordion (only it's reference is updated)
Screen.Recording.2025-01-08.at.10.04.47.AM.mov
|
I opened #692 in response to the behaviour above. Workspace operations that don't go through textEdit/did{open,change,close} don't update the document manager (e.g. a As for syntax errors, that's a tough one. I think the example we have above is tricky because we're working of a theme that is doing internal explorations of features. It's technically not valid in the wild... But the space between "might be vaild" and "not valid" is pretty large. I'm not sure it's worth looking into? |
| * 4. References in {% content_for "block", type: "oldName" %} must be changed | ||
| * | ||
| * Things we're not doing: | ||
| * 5. If isPublic(oldName) && isPrivate(newName) && "schema.blocks" accepts "@theme", |
There was a problem hiding this comment.
This is a really good catch - didn't think we'd be doing it but good to explicitly have this written out
| } | ||
|
|
||
| private getSchemaChanges( | ||
| sectionsWithLocalBlocks: Set<unknown>, |
There was a problem hiding this comment.
instead of passing in this set as an arg and modifying it in this function, could we just create the set and return from within the function?
There was a problem hiding this comment.
This function already returns a function though, not sure how I feel about returning a bunch of things (à la useState)
Which would make this invocation kind of awkward require two calls anyway?
There was a problem hiding this comment.
Moreover, the object would only be full after the promise resolves... Doesn't that make it more complicated?
// We need to keep track of sections that have local blocks, because we
// shouldn't rename those. Only uses of "@theme" or specifically named blocks
// should be renamed when the blocks/*.liquid file is renamed.
const [sectionsWithLocalBlocks, getSchemaChanges] = this.getSchemaChanges(oldBlockName, newBlockName);
// sectionWithLocalBlocks is empty right now
const sectionAndBlocksChanges: (DocumentChange | null)[] = await Promise.all(sectionsAndBlocks.map(getSchemaChanges));
// sectionWithLocalBlocks is full
```There was a problem hiding this comment.
This function already returns a function though, not sure how I feel about returning a bunch of things (à la useState)
👍
packages/theme-language-server-common/src/renamed/handlers/BlockRenameHandler.ts
Outdated
Show resolved
Hide resolved
What are you adding in this PR?
When a theme block gets renamed, old references will be workspace edited to the new name in all the following locations:
content_for "block", type: "old-name"templates/*.jsonfilessections/*.jsonfileson-block-rename.mp4
Before you deploy
changeset