-
Notifications
You must be signed in to change notification settings - Fork 26
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
Auto-complete metafields depending on shop data #534
Conversation
0794386
to
c17af0c
Compare
b740a20
to
ec1edd6
Compare
ec1edd6
to
77e9f57
Compare
...eme-language-server-common/src/completions/providers/HtmlAttributeCompletionProvider.spec.ts
Outdated
Show resolved
Hide resolved
...ages/theme-language-server-common/src/completions/providers/ObjectCompletionProvider.spec.ts
Show resolved
Hide resolved
packages/theme-language-server-common/src/hover/providers/LiquidObjectHoverProvider.spec.ts
Outdated
Show resolved
Hide resolved
packages/theme-language-server-common/src/hover/providers/LiquidObjectHoverProvider.spec.ts
Show resolved
Hide resolved
packages/theme-language-server-common/src/server/startServer.ts
Outdated
Show resolved
Hide resolved
packages/theme-language-server-common/src/server/Configuration.ts
Outdated
Show resolved
Hide resolved
packages/theme-language-server-common/src/server/startServer.ts
Outdated
Show resolved
Hide resolved
|
77e9f57
to
a0e15fc
Compare
packages/theme-language-server-common/src/server/startServer.ts
Outdated
Show resolved
Hide resolved
...theme-language-server-common/src/completions/providers/TranslationCompletionProvider.spec.ts
Outdated
Show resolved
Hide resolved
...eme-language-server-common/src/completions/providers/RenderSnippetCompletionProvider.spec.ts
Outdated
Show resolved
Hide resolved
...ages/theme-language-server-common/src/completions/providers/FilterCompletionProvider.spec.ts
Outdated
Show resolved
Hide resolved
...anguage-server-common/src/completions/providers/HtmlAttributeValueCompletionProvider.spec.ts
Outdated
Show resolved
Hide resolved
packages/theme-language-server-common/src/server/startServer.ts
Outdated
Show resolved
Hide resolved
a0e15fc
to
7e5722a
Compare
|
7e5722a
to
3cea5c3
Compare
|
||
if (!metafieldsProperty) continue; | ||
|
||
metafieldsProperty.return_type = [{ type: `${category}_metafields`, 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.
Hmm. Thinking about this, are we sure we're not mutating the original object here? Should we clone/replace metafieldsProperty before doing this mutation?
Remember that ...objectMap
gives you a shallow clone of the object map. Looks like we're mutating the original object map here and this is no bueno. Means that if you'd get metafield data from one theme in another.
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.
Looks like we're mutating the original object map here and this is no bueno. Means that if you'd get metafield data from one theme in another.
OH i didn't know that! Yeah this is actually modifying existing type's properties. E.g. product
would be modified so that it's metafields
property would return product_metafields
instead of untyped
.
I guess i could clone product
, modify its property, and add it to result
👍
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.
We talked about it in person - since this isn't unique to a theme, we should be fine with this change happening across themes within a workspace. Added a warning message in a comment above the block instead.
...new Set([...Object.values(METAFIELD_TYPE_TO_TYPE), ...FETCHED_METAFIELD_CATEGORIES]), | ||
].reduce((map, type) => { | ||
{ | ||
const metafieldEntry = JSON.parse(JSON.stringify(baseMetafieldEntry)); // easy deep clone |
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.
Oh are we doing it already. I'll just dm you to verify :D this code is dense and hard to review lol
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.
Yeah sorry there are three things happening to make the whole thing work. We can talk about it more today, but here are the things:
customMetafieldTypeEntries
--> we copy themetafield
type and create one for each category we support. e.g.product_metafield
,collection_metafield
, etc.metafieldDefinitionsObjectMap
--> reads the metafield definitions we fetched and converts it into types for auto-completion- the for-loop you commented above --> we override existing types like
product
andcollection
, so theirmetafields
properties don't return asuntyped
, but instead returnproduct_metafields
andcollection_metafields
respectively
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.
LGTM, didn't tophat yet 👀
1b68cbc
to
44eff4c
Compare
44eff4c
to
58bdfaf
Compare
What are you adding in this PR?
Part 1 for #502
.shopify
folderWhat's next? Any followup issues?
What did you learn?
DidChangeWatchedFilesNotification
took some time, but had to read-up on client capabilities and how to subscribe to notifications after initializationhttps://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_didChangeWatchedFiles
Tophat
Run the following query and dump the contents into
.shopify/metafields.json
in the theme repository. Remove thenodes
array, and place it directory under each group.Before you deploy
changeset