Update the asset ignore module to be backward-compatible

karreiro commented 18 hours ago

WHY are these changes introduced?

Fixes https://github.com/Shopify/cli/issues/4496

WHAT is this pull request doing?

This PR updates the ignore module (--only/--ignore/.shopifyignore) to be backward compatible with (Ruby) Shopify CLI 2. The infrastructure code is no longer opinionated about minimatch defaults, and the themes module passes matchBase=true and noglobstar=true.

How to test your changes?

Run shopify theme init test
Run cd test
Create .shopifyignore file with templates/**/*.json
Run shopify theme push -d --verbose
Run shopify theme push --legacy --verbose
Notice the ignored files matches now

Notice: in the original report the user reports this issue using the --password flag instead of --legacy. Both flags activate the legacy implementation, but --legacy is more straightforward because it doesn't require any password. The reason the --password flag uses the legacy implementation is that this flag is designed to be used in CI/CD environments, so we decided to keep running the legacy implementation there in the first release of commands.

Post-release steps

N/A

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
[x] Existing analytics will cater for this addition
[ ] PR includes analytics changes to measure impact

Checklist

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

github-actions[bot] commented 18 hours ago

Thanks for your contribution!

Depending on what you are working on, you may want to request a review from a Shopify team:

Themes: @shopify/advanced-edits
UI extensions: @shopify/ui-extensions-cli
- Checkout UI extensions: @shopify/checkout-ui-extensions-api-stewardship
Hydrogen: @shopify/hydrogen
Other: @shopify/app-inner-loop

github-actions[bot] commented 18 hours ago

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

```diff @@ -283,6 +283,7 @@ export declare function pathToFileURL(path: string): URL; export declare function findPathUp(matcher: OverloadParameters[0], options: OverloadParameters[1]): ReturnType; export interface MatchGlobOptions { matchBase: boolean; + noglobstar: boolean; } /** * Matches a key against a glob pattern. ```

github-actions[bot] commented 18 hours ago

Coverage report

St.^{:grey_question:}	Category	Percentage	Covered / Total
🟡	Statements	72.61% (+0.01% 🔼)	8522/11737
🟡	Branches	69.55% (+0.03% 🔼)	4178/6007
🟡	Functions	71.61%	2205/3079
🟡	Lines	72.92% (+0.01% 🔼)	8064/11058

Test suite run success

1939 tests passing in 873 suites.

Report generated by 🧪jest coverage report action from d9fff2c30ce1b0fcc32afab8b743a85f43567053

jamesmengo commented 7 hours ago

I think this behaves differently when there are > 1 levels such as templates/customers/test/file.liquid.

Not a valid directory structure in today's world, though maybe one day it will be

Shopify / cli