[Themes] Improve Glob Pattern subdirectory mismatch error handling #3669
Conversation
This comment has been minimized.
This comment has been minimized.
jamesmengo
added
Type: Bug
Coverage report
Show files with reduced coverage 🔻
Test suite run success1617 tests passing in 754 suites. Report generated by 🧪jest coverage report action from e1f01b0 |
jamesmengo
force-pushed
the
jmeng/improve_glob_pattern_warning
branch
from
88a7d2d
to
72a46ba
Compare
There was a problem hiding this comment.
Thank you for this PR, @jamesmengo! I've left only one minor question, please let me know what you think about it :)
| const newPatternMatch = originalMatchGlob(key, pattern.replace('/*.', '/**/*.')) | ||
| if (newPatternMatch) { |
There was a problem hiding this comment.
Personally, I don't think we should adopt pattern matching or not matching as our criteria for showing the warning, as the pattern is still used when it's not being matched.
What do you think about moving this warning to the applyIgnoreFilters level? Then, we'd display this warning once per pattern (instead of per pattern usage), and we'd still inform users about this :)
jamesmengo
force-pushed
the
jmeng/improve_glob_pattern_warning
branch
from
302fee0
to
8ca5366
Compare
jamesmengo
force-pushed
the
jmeng/improve_glob_pattern_warning
branch
from
8ca5366
to
e1f01b0
Compare
| }) | ||
| } | ||
|
|
||
| function shouldReplaceGlobPattern(pattern: string): boolean { |
There was a problem hiding this comment.
Decision: I chose to limit this to the templates folder because that's the only folder where subdirectories are supported.
Some questions that could influence this
-
Do you consider something like
assets/*.liquidto be unsafe / something we need to substitute?- If we recommend
assets/**/*.liquid, that could be confusing since there are no subdirectories there
- If we recommend
-
Will we remove the automatic conversion of the glob pattern one day so that
templates/*.jsonandtemplates/**/*.jsonmean different things in the future?- Some context here
There was a problem hiding this comment.
+1 for restricting this warning to templates, as it's the only directory that may have a sub-directory :)
Regarding the removal of this warning in the future, because it would be a destructive breaking change (as in some contexts it may imply file deletion), I'd suggest making this change in the next major release
| }) | ||
| } | ||
|
|
||
| function shouldReplaceGlobPattern(pattern: string): boolean { |
There was a problem hiding this comment.
+1 for restricting this warning to templates, as it's the only directory that may have a sub-directory :)
Regarding the removal of this warning in the future, because it would be a destructive breaking change (as in some contexts it may imply file deletion), I'd suggest making this change in the next major release
WHY are these changes introduced?
This warning was added to surface some work that we were always doing on the Ruby side to include subdirectories when pattern matching theme files (
.shopifyignore,--ignore,--only)Technically, the pattern
templates/*.jsonshould not match subdirectories, but this would be a largely breaking change, so we apply this change on the platform side when needed.WHAT is this pull request doing?
How to test your changes?
templates/customers)pnpm shopify theme push --path <PATH_TO_THEME> -u --ignore "templates/*.json"--onlyor.shopifyignoretemplates/**/*.jsonand observe that the messages disappearPost-release steps
Measuring impact
How do we know this change was effective? Please choose one:
Checklist
devordeployhave been reflected in the internal flowchart.