Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Themes] Simplify ThemeFileSystem.files default values #4761

Open
jamesmengo opened this issue Oct 28, 2024 · 0 comments
Open

[Themes] Simplify ThemeFileSystem.files default values #4761

jamesmengo opened this issue Oct 28, 2024 · 0 comments
Labels
Area: @shopify/theme @shopify/theme package issues tech debt Liquid SF (tech debt)

Comments

@jamesmengo
Copy link
Contributor

jamesmengo commented Oct 28, 2024

ThemeFileSystem.files have an attachment, or a value

Currently, we save '' instead of undefined. For example, images would have:
{attachment: '<base64_string>, value: ''}

We should either update the type signature to reflect this, or update the default value to set undefined

Suggestions

  • We can consider consistency with the API or simplicity of CLI code when making this decision.
  • Personally, I think modelling how the API views this is a good first pass approach. We should set the empty value to undefined and verify any fallback logic of || or ?? doesn't break. (This could break if we are expecting the '' value in the LHS of || to be considered truthy.

          This should stay `||` imo. This will reflect the behaviour in [theme-fs](https://github.com/Shopify/cli/blob/55307bb53694f67022862d84de5f5e85744feec7/packages/theme/src/cli/utilities/theme-fs.ts#L60-L75), which saves `''` instead of `undefined` 

Now that I think about it, we could probably consider changing that across the codebase (should be done separately if so)

Originally posted by @jamesmengo in #4732 (comment)

@jamesmengo jamesmengo added Area: @shopify/theme @shopify/theme package issues tech debt Liquid SF (tech debt) labels Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: @shopify/theme @shopify/theme package issues tech debt Liquid SF (tech debt)
Projects
None yet
Development

No branches or pull requests

1 participant