Render warning when using folder/ directory pattern

[EvilGenius13]

This commit fixes an issue in the matchGlob to match directories when filtering through files

WHY are these changes introduced?

References: #4855

There is some confusion on how patterns work when specifying folders with the --only or --ignore flags. This is partly due to the change from Ruby to Typescript, and the way directory patterns are matched. See my comment for more information.

WHAT is this pull request doing?

Added a small renderWarning when someone uses a folder pattern that ends with a / such as /templates and gives two ideas for patterns that will also grab nested folders/files.

Example

Message:

When using `/templates/*:

CLI.Message.Warning.mp4

How to test your changes?

Pull down the branch
Build the branch
If in a directory with a theme:
- delete two different folders like assets and templates
- run shopify theme pull -t <your_thene> -o templates/
- You should see the warning message
If in a blank directory:
- run shopify theme pull -t <your_theme> -o templates/
- You should see the warning message

Post-release steps

Measuring impact

How do we know this change was effective? Please choose one:

• n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
• Existing analytics will cater for this addition
• PR includes analytics changes to measure impact

Checklist

• I've considered possible cross-platform impacts (Mac, Linux, Windows)
• I've considered possible documentation changes

[github-actions]

Coverage report

St.❔	Category	Percentage	Covered / Total
🟡	Statements	75.32% (+0% 🔼)	8889/11801
🟡	Branches	70.56% (+0.02% 🔼)	4322/6125
🟡	Functions	75.1%	2337/3112
🟡	Lines	75.86% (+0.01% 🔼)	8401/11074

Show files with reduced coverage 🔻

St.❔	File	Statements	Branches	Functions	Lines
🟢	... / app-event-watcher.ts	93.83% (-1.23% 🔻)	86.49%	90.48%	98.61%
🟡	... / build.ts	78.51% (-0.18% 🔻)	64.58%	78.38%	76.99% (-0.2% 🔻)

Test suite run success

2001 tests passing in 904 suites.

Report generated by 🧪jest coverage report action from 3c18d50

[jamesmengo]

can you test this with templates/ to test a nested folder structure? I think it'll break

[jamesmengo]

🤔
Thanks for this change! I may be bike-shedding here, but I want to raise some thoughts I have about this approach.

The issue here arises because we don't have a strong glob contract.

• We're now using minimatch as our matching library after the TS rewrite
• For backwards compatibility reasons, we try to silently fix / replicate Ruby glob behaviour as a fallback [2]

My personal preference here is to diverge as little as possible from the JavaScript glob matching library moving forward.

I think we could render a warning to get the user to fix/remediate this issue rather than adding more custom behaviour.
ALTERNATIVELY, we can raise the warning / suggestion, but still fix it for them if needed by following the pattern laid out in [2].

As always, open to discussion and changing my mind!

[2]

• That's why we automatically substitute **/* here, since users already have globs that they're relying on.

We needed to do so because the Ruby library that we were previously using behaves differently than the javascript glob matcher 🤢

[karreiro]

Thank you for this PR, @EvilGenius13!

I resonate with the motivation behind this work. The --only/--ignore flags have an unusual behavior to maintain backward compatibility with the previous Ruby version.

When the Ruby CLI was migrated to the TypeScript CLI, many developers were affected by changes in this module, that was shape to avoid as much friction as possible.

Considering the change in behavior here, I would suggest waiting for a major CLI update to proceed with this approach, finally break backward-compatibility, and establish a more consistent ignore mechanism.

With that context in mind, I believe we could follow the approach suggested by @jamesmengo on this PR.

To avoid overwhelming users with warning messages, we could check the shape of the flag (as you are already doing) and the files being ignored (or selected by --only). If an --ignore flag is not effectively ignoring anything, and the shape indicates the scenario this PR addresses, we have an opportunity to warn developers.

In the warning, we could encourage users to use regexes (instead of globs) for better results, so the shape of the directory doesn't impact the pattern.

I understand this PR implements an improved behavior, but considering the potential impact, I believe this is the best path to prevent friction in a minor release.

Thanks again for this PR!

[EvilGenius13]

@jamesmengo @karreiro Thank you for the feedback! I will hold off for now. I am trying a few options, but I believe to properly make the nested folder pattern function properly, we will have to break backwards-compatibility.

[github-actions]

Differences in type declarations

We detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:

• Some seemingly private modules might be re-exported through public modules.
• If the branch is behind main you might see odd diffs, rebase main into this branch.

New type declarations

We found no new type declarations in this PR

Existing type declarations

packages/cli-kit/dist/public/node/fs.d.ts

@@ -314,6 +314,7 @@ export declare function findPathUp(matcher: OverloadParameters<typeof internalFi
 export interface MatchGlobOptions {
     matchBase: boolean;
     noglobstar: boolean;
+    dot?: boolean;
 }
 /**
  * Matches a key against a glob pattern.

[jamesmengo]

@EvilGenius13 FWIW, I believe templates/ would have worked in the Ruby CLI to match nested folders, so this is technically a breaking change for those users.

However, since there are now users who may have started using the asset-ignore logic after we switched to TS, we would be now adding a breaking change for them.

So, to fix forward, we should try to get the Ruby users to update their patterns by making them aware when this happens, so like Guilherme suggested, we can render a warning if:

• pattern ends in /
• no matches
• folder is not empty

It's definitely a tricky scenario, so we can chat about this more! :)

[aleksandar89d]

It is still not possible to pull only specific folder? Example shopify theme pull -o templates/

[github-actions]

We detected some changes at packages/*/src and there are no updates in the .changeset.
If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.

[EvilGenius13]

Thank you for all the feedback. After doing some more digging and pairing with @jamesmengo, it's best to simply make a warning to give more information on a proper pattern.

Based on TypeScript behaviour, using a pattern such as templates/ will only return the top level folder.

In order to grab nested folders/files, the pattern would be one the following:

1. If someone wants to include nested folders, they would use templates/*
2. If someone wants to include nested folders with a specific file type, they would use templates/*.liquid

If someone types in /templates a small output will return with a message stating to use option 1 or 2.

This also leaves the backwards compatible checks in place from the ruby implementation.

[jamesmengo]

@aleksandar89d you can use templates/* to match the entire templates folder