add deeplinking for block types #674
Conversation
itsjustriley
force-pushed
the
deeplink-theme-blocks
branch
from
352ac64 to
6d5ccd2
Compare
| documentManager.open(uriString, liquidRawTagContent, 1); | ||
|
|
||
| const result = await documentLinksProvider.documentLinks(uriString); | ||
| expect(result).toEqual([]); |
There was a problem hiding this comment.
The test case description suggests it should return a list of document links with correct URLs, but the expectation is an empty array. This should be corrected to check for the correct URLs.
| expect(result).toEqual([]); | |
| expect(result).toEqual(expectedUrls); |
| } | ||
| }); | ||
|
|
||
| it('should return a list of document links with correct URLs for a LiquidRawTag document', async () => { |
There was a problem hiding this comment.
The name of this unit test and what its verifying for doesnt quite match. I do think that the test case you are describing here should be tested in addition to the empty array case you implemented for.
| ); | ||
| } | ||
|
|
||
| function processPresetBlocks( |
There was a problem hiding this comment.
This is kind of a hefty function. it would help with scanability to encapsulate common functionality between the conditional branches.
|
|
||
| if (blocksNode.type === 'object' && blocksNode.children) { | ||
| blocksNode.children.forEach((propertyNode) => { | ||
| const blockValueNode = propertyNode.children?.[1]; // The value node of the property |
There was a problem hiding this comment.
Its always the second argument of the children array?
| } | ||
|
|
||
| const typeNode = findNodeAtLocation(blockValueNode, ['type']); | ||
| if (typeNode && typeNode.type === 'string' && typeof typeNode.value === 'string') { |
There was a problem hiding this comment.
When evaluating a generic variable conforms to a specific structure, you can do something called "type narrowing" when there is a specific type definition for the structure of data that you're verifying for. This has typescript benefits of showing proper typing feedback as you write code inside these conditionals. Take a look at these examples of type narrowing code in the theme-tools repo: https://github.com/Shopify/theme-tools/blob/deeplink-theme-blocks/packages/theme-check-common/src/types.ts#L23-L25
| } | ||
|
|
||
| const typeNode = findNodeAtLocation(blockNode, ['type']); | ||
| if (typeNode && typeNode.type === 'string' && typeof typeNode.value === 'string') { |
There was a problem hiding this comment.
same comment about type narrowing this to something more specific from JSONNode
| } | ||
| }); | ||
| } else if (blocksNode.type === 'array' && blocksNode.children) { | ||
| blocksNode.children.forEach((blockNode) => { |
There was a problem hiding this comment.
One way to reduce this big function is to move the iteree code within the forEach into a standalone function definition.
There was a problem hiding this comment.
Awesome start, Riley! The logic is looking really good!
I wanted to make a quick suggestion as you streamline the code, which could make your deeplinking implementation a bit more robust. CP recently made an update that could simplify the parsing process.
By using this approach, you could, for instance, collect all the blocks initially and then add in the links. This method may also help with type checking.
If you're curious about how this works, you can check out this parsing in action here.
Looking forward to the release! 😄
3 participants
Draft PR for #631
This is functional, but not streamlined. There's probably some logic here that can consolidated into helper functions, and there may be an easier way to go through the blocks/types.
But this works for top level block types, and block types in presets. As intended, it does not deeplink inline block types or ones starting with @.