Shopify / theme-tools

Everything developer experience for Shopify themes
https://shopify.dev/docs/themes
MIT License
78 stars 24 forks source link

Add config exceptions to Remote Asset check #410

Open eloyesp opened 2 years ago

eloyesp commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Good argument. Yeah ok!