Shopify / cli

Build apps, themes, and hydrogen storefronts for Shopify
https://shopify.dev
MIT License
433 stars 130 forks source link

Theme check: command doesn't inspect any file after CLI upgrade #3497

Closed ivansimplistic closed 8 months ago

ivansimplistic commented 8 months ago

Describe the bug Running the command line 'shopify theme check' is not inspecting files, returning a Success message , 0 files inspected with no offenses.

Source It happens on all Theme projects, even Dawn. It also happens to my other colleagues on their computers.

Expected behaviour The command should output a large list of errors and suggestions.

Actual behaviour The command is not inspecting files. The command returns: Success Theme Check Summary. 0 files inspected with no offenses found.

Debugging information

Additional context The command was working fine until I upgraded shopify CLI from v3.48.x to v3.52.x. It is broken since then.

See an example with a fresh start of a Dawn theme: image

charlespwd commented 8 months ago

I think we somehow made the path argument required as a mistake. I believe there's a fix coming to the next release.

How about shopify theme check --path .? Same thing or no?

Also, is there a .git in that folder?

Don't you need to dir into the theme before running the command?

charlespwd commented 8 months ago

Seems to be working for me 🤔 image

ivansimplistic commented 8 months ago

How about shopify theme check --path .? Same thing or no? Same thing, and it appears in the .theme-check.yml file. I also tried with .\, and ./ but there is no difference

Also, is there a .git in that folder? Yes there is, it was created by the shopify theme init command. I tried removing the folders .git, .github, .vscode and running theme check again, but still get the same result.

Don't you need to dir into the theme before running the command? Sorry, I forgot the cd into dir on the example. But anyway, I did it now and still get the same result.

New info from my colleagues, it seems that the issue happens only on Windows. Theme check still works on their Mac computers.

image

charlespwd commented 8 months ago

Ha that's interesting. Thanks for the info. Reopening. We should see what's up with windows. Transferring to CLI since this seems to be a CLI issue. Does the VS Code extension work fine in that repo?

https://marketplace.visualstudio.com/items?itemName=Shopify.theme-check-vscode

ivansimplistic commented 8 months ago

Yes, the extension works fine in that repo. I don't know if I can make it check all the files in the theme, instead of just the opened files.

About a couple of weeks ago the vscode extension started showing new errors in our code, like HTML tags that were not closed correctly, or unused variables. This was great, but unfortunately also started to show way too many false errors:

Know that those false positives only appear on some of the repos. I don't know if there is something in the .theme-check.yml hiding the errors on other repos. I suppose there isn't.

charlespwd commented 8 months ago

Unknown object 'section' used.theme-checkUndefinedObject (inside section files)

This usually happens because of not specifying the root property in .theme-check.yml. If you have a src folder, add this to your .theme-check.yml

root: ./src

That or you don't have a .git.

The other problem is the same thing. Because we can't infer the root, we can't find the translations and that breaks the translation check.

ivansimplistic commented 8 months ago

Thanks! That fixed those checks made by the extension.

The failing repos didn't even had a .theme-check.yml. Although the root of the theme was the root of the vs code project, so it would be great if the extension could run using root: . by default. Anyway, now I know how to solve it in the future, ty.

It worked regardless of the existence of .git

charlespwd commented 8 months ago

Tagged the wrong issue with PR

justshrey commented 8 months ago

Ok, I had the same issue and have figured out the problem.

Basically, the issue is in the theme-check-node > src > index.ts - getTheme function. On windows, the glob function uses has an issue with the path separator i.e '\' - glob fails to pick up any files from the folders at all. Have fixed it below - just have to replace the getTheme function.

Will raise a PR shortly once I test it end to end on windows.

export async function getTheme(config: Config): Promise<Theme> {

  // On windows machines - the separator provided by path.join is '\' 
  // however the glob function fails silently since '\' is used to escape glob character
  // as mentioned in the documentation of node-glob

  // the path is normalised and '\' are replaced with '/' and then passed to the glob function
  const normalized_path = path.normalize(path.join(config.root, '**/*.{liquid,json}')).replace(/\\/g, '/');

  const paths = await asyncGlob(normalized_path).then((result) =>
    // Global ignored paths should not be part of the theme
    result.filter((filePath) => !isIgnored(filePath, config)),
  );
  const sourceCodes = await Promise.all(paths.map(toSourceCode));
  return sourceCodes.filter((x): x is LiquidSourceCode | JSONSourceCode => x !== undefined);
}