Gracefully handle files that are not from a theme #985
Conversation
1166733 to
6c8ebaf
Compare
| const path = join( | ||
| await findRoot(context.file.uri, context.fileExists), | ||
| 'config/settings_schema.json', | ||
| ); | ||
| const settingsFile = await context.fs.readFile(path); |
There was a problem hiding this comment.
toURI does this better.
| const result: FileTree = {}; | ||
| for (const [relativePath, source] of Object.entries(this.mockTheme)) { | ||
| const segments = relativePath.split('/'); | ||
| let current = result; | ||
| for (let i = 0; i < segments.length 8000 - 1; i++) { | ||
| const segment = segments[i]; | ||
| current[segment] ??= {}; | ||
| current = current[segment] as FileTree; | ||
| } | ||
| current[segments[segments.length - 1]] = source; | ||
| } | ||
| return this._fileTree; | ||
| return result; |
There was a problem hiding this comment.
Had to remove memoization because of a test that mutated this.mockTheme. It's only for tests, perf doesn't matter :)
| @@ -43,7 +43,7 @@ export function makeRunChecks( | |||
| // then we recheck theme1 | |||
| const fileExists = makeFileExists(fs); | |||
| const rootURIs = await Promise.all(triggerURIs.map((uri) => findRoot(uri, fileExists))); | |||
| const deduplicatedRootURIs = new Set(rootURIs); | |||
| const deduplicatedRootURIs = new Set<string>(rootURIs.filter((x): x is string => !!x)); | |||
| await Promise.all([...deduplicatedRootURIs].map(runChecksForRoot)); | |||
There was a problem hiding this comment.
Impact: runChecks won't run when triggered from a file that you can't find a root for.
| if (!rootUri) { | ||
| return []; | ||
| } | ||
|
|
There was a problem hiding this comment.
No document links when you can't find a root.
| @@ -71,7 +72,7 @@ export class LiquidVariableRenameProvider implements BaseRenameProvider { | |||
| const rootUri = await this.findThemeRootURI(params.textDocument.uri); | |||
| const textDocument = document?.textDocument; | |||
|
|
|||
| if (!textDocument || !node || !ancestors) return null; | |||
| if (!rootUri || !textDocument || !node || !ancestors) return null; | |||
There was a problem hiding this comment.
No variable rename when you can't find a root.
| @@ -44,6 +45,7 @@ export class AssetRenameHandler implements BaseRenameHandler { | |||
| if (relevantRenames.length !== 1) return; | |||
| const rename = relevantRenames[0]; | |||
| const rootUri = await this.findThemeRootURI(path.dirname(params.files[0].oldUri)); | |||
| if (!rootUri) return; | |||
There was a problem hiding this comment.
No asset rename when you can't find a root
| ) {} | ||
|
|
||
| async getThemeGraphForURI(uri: string) { | ||
| const rootUri = await this.findThemeRootURI(uri); | ||
| if (!rootUri) { | ||
| return undefined; |
There was a problem hiding this comment.
No theme graph for files that are not part of a theme
| @@ -58,7 +58,7 @@ describe('Module: server', () => { | |||
| } | |||
| }); | |||
|
|
|||
| fileTree = { 'snippets/code.liquid': fileContents }; | |||
| fileTree = { '.theme-check.yml': '', 'snippets/code.liquid': fileContents }; | |||
There was a problem hiding this comment.
Since Application Support/Code/User/snippets exists and since Application Suport/Code/User/settings.json is a file you're likely to edit, we were inferring Application Support/Code/User to be the root of a theme. That was not very good. So I changed the algo to look for snippets and assets (as well as looking for .theme-chekc.yml).
For the unit tests, it was easiest to simply put a .theme-check.yml
| if (!rootUri) { | ||
| return {} as MetafieldDefinitionMap; | ||
| } |
There was a problem hiding this comment.
no root URI means no .shopify folder gets generated in non-themes.
There was a problem hiding this comment.
Metafields file never got created on my system but i feel like it's a problem outside of the rootUri... can you check yours to see if it's working. It's failing on the exec function in packages/theme-language-server-node/src/metafieldDefinitions.ts line 46.
There was a problem hiding this comment.
They load fine for me 🤔
| if (!rootUri) { | ||
| return {} as MetafieldDefinitionMap; | ||
| } |
There was a problem hiding this comment.
Metafields file never got created on my system but i feel like it's a problem outside of the rootUri... can you check yours to see if it's working. It's failing on the exec function in packages/theme-language-server-node/src/metafieldDefinitions.ts line 46.
Make findThemeRootURI return null when it can't find a root Make everywhere handle null when no root is found Should fix "preloading" of theme when looking at `$HOME/Library/Application Support/Code/User/settings.json` Fixes #973
6c8ebaf to
d592591
Compare
Make findThemeRootURI return null when it can't find a root
Make everywhere handle null when no root is found
Should fix "preloading" of theme when looking at
$HOME/Library/Application Support/Code/User/settings.jsonAlso,
snippetscan be found in$HOME/Library/Application Support/Code/Userand is a VS Code-owned, non theme thing. So had to modify the findRoot algo to search for assets and snippets folder existence.Fixes #973
Before you deploy
changeset