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

Add config exceptions to Remote Asset check #410

Open
eloyesp opened this issue Aug 31, 2022 · 3 comments · May be fixed by #711
Open

Add config exceptions to Remote Asset check #410

eloyesp opened this issue Aug 31, 2022 · 3 comments · May be fixed by #711
Assignees
Labels

Comments

@eloyesp
Copy link

eloyesp commented Aug 31, 2022

Is your feature request related to a problem? Please describe.

There are valid exceptions for the single CDN domain that might repeat a lot, and it would be nice to document those on the config file, my personal example now is with polyfill.io that does some magic not available on shopify CDN and is not beauty to add exceptions comments on each place. Other example would be a shop that choose to use Cloudinary across the theme, it is clearly better if the config file could make clear what remotes are "valid" for the store.

Describe the solution you'd like

RemoteAsset:
  additional_remotes:
    - polyfill.io

Describe alternatives you've considered

Using disable comments solves the issue when most violations can be made on the same file, but it adds too much noise when violations are expected on many files.

Additional context

I've been reading the implementation of the check, and it seems that adding this feature seems to be pretty straightforward.

@charlespwd
Copy link
Contributor

charlespwd commented Aug 31, 2022

Other example would be a shop that choose to use Cloudinary across the theme, it is clearly better if the config file could make clear what remotes are "valid" for the store.

But that's the thing we want to discourage! https://cdn.shopify.com is an image transformation and compression service (see its home page!).

Moreover, any kind of incremental gains you'd get from using cloudinary would be dwarfed by the TCP slow start of the new connection. Plus, HTTP2 prioritization does not play nice on two domains. HTTP connections have a cost!

More details can be found in the docs of the check.

As for polyfill.io, while it's true that our CDN doesn't support it, I feel like a singular {% # theme-check-disable RemoteAsset %} comment is fine. It demonstrates that you care and that what you are doing is intentional.

I'm not sure removing the friction is the way to go if we want to encourage best practices.

@eloyesp
Copy link
Author

eloyesp commented Aug 31, 2022

But that's the thing we want to discourage! https://cdn.shopify.com is an image transformation and compression service (see its home page!).

And that is great for most shops out there, but most theme have exceptions, because services like Cloudinary are much more than just that (IA enhanced crop, effects, transparency, etc.).

Even with that, Cloudinari was just an example, all the shops have different requirements, and that might involve adding resources on different domains, there are thousands of shops out there.

It is much better to let users add an exception than forcing users that need to do something that they know is not as performant, but also know that they need that under circumstances unknown for the "theme-check" developer, to disable the rule entirely.

If users end up disabling rules that get on the way of the shop requirements, developers will eventually add more domains (by mistake) and the result will be even worse.

Theme check will obviously not match all the situations that are possible, defaults should adapt to the most common escenarios, configuration should allow to adapt to most scenarios.

@charlespwd
Copy link
Contributor

Good argument. Yeah ok!

@lukeh-shopify lukeh-shopify transferred this issue from Shopify/theme-check Jul 12, 2024
@charlespwd charlespwd added the good first issue Good for newcomers label Oct 17, 2024
@zacariec zacariec self-assigned this Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants