search

Shopify / theme-check

The Ultimate Shopify Theme Linter
Other
333 stars 96 forks source link

Theme check corrects `nil` to `null` without the config option to override this setting #720

Closed vfonic closed 1 year ago

vfonic commented 1 year ago

Describe the bug

Hi,

Liquid template language has been using (Ruby's) nil since the day it was released. I don't know when exactly null was introduced and was it there from the beginning, but I'm do know that nil was the keyword that was being used until a few months ago. I remember that @charlespwd posted a poll somewhere whether theme-check should prefer null or nil and the majority of the votes were in favor of null. I'm not.

I think nil would be better, because:

  1. It was used since forever
  2. It has VS Code syntax support (null (still) doesn't)
  3. Liquid is not JavaScript. It's a different programming language with its own syntax.
  4. All existing Liquid code snippets using nil need to be autocorrected. No one, until it was switched as default autocorrect, wasn't using null*

Some examples: https://github.com/EvgeniyMukhamedjanov/liquid-ajax-cart/blob/c9e76bf4054ed691cb5268cc995645eb93888382/sections/cart-template.liquid#L80 https://github.com/EvgeniyMukhamedjanov/liquid-ajax-cart/blob/428c821b829f4bb88f2d2e5b0b1a4c084a1ab628/templates/customers/order.liquid#L39 Here someone actually uses null: https://github.com/Cam/Shopify-Theme-Framework/blob/bb13b0f38ea3d4ff7278f5214225428da74603e9/snippets/related-products.liquid#L5 But then, within the same repo: https://github.com/Cam/Shopify-Theme-Framework/blob/bb13b0f38ea3d4ff7278f5214225428da74603e9/templates/account.liquid#L15 https://github.com/Cam/Shopify-Theme-Framework/blob/bb13b0f38ea3d4ff7278f5214225428da74603e9/templates/customers/account.liquid#L13 This is just creating a mess... https://github.com/barrel/shopify-vite/blob/f7059ba88e037b5ca017afaa93cd9cb1f2028a55/themes/seed-theme/snippets/structured-data.liquid#L26 https://github.com/barrel/shopify-vite/blob/f0bd456ae87618bbb5266f7da0093e068662a30b/themes/seed-theme/snippets/collection-filters.liquid#L176 https://github.com/barrel/shopify-vite/blob/391c12519347b5f3e5d8de3f454b8bf13e6fff19/themes/seed-theme/snippets/cart-item.liquid#L15 Dawn has both nil and null because Dawn theme code doesn't have Prettier fully setup (what a pity): https://github.com/Shopify/dawn/search?q=null https://github.com/Shopify/dawn/search?q=nil

*if it's not obvious "no one" means "a lot of people didn't"

Expected

nil is used in Liquid as that's what has been used since forever and was working just fine.

Or

At least allow me to change the default.

Actual

I'm forced to switch to null because now that's the default after some poll was posted somewhere and we decided to change it for everyone.

If I'm mistaken and there's a config option, please guide me: https://github.com/Shopify/theme-check/tree/main/docs/checks

Also, it would be great to have a docs page like this one: https://prettier.io/docs/en/options.html

Instead of readme pointing to the list of individual markdown files: https://github.com/Shopify/theme-check/tree/main/docs/checks

charlespwd commented 1 year ago

This is an issue that was brought up repeatedly. We might make a config for it.

As per our opinionated principle, we only want to add configs if the setting is truly controversial. If you don't do this, you end up with a config mess and a code mess. Simplicity matters. You are free to open a discussion/issue in the prettier-plugin-liquid repo and gather interest for a config for it. That's welcomed and encouraged!

As for the docs, you don't find it here because the issue was reported on the wrong repo. The formatter lives in the prettier-plugin-liquid repository, so its configuration options are also documented there.

As per null over nil, they both resolve to the same value. The goal was to swap one for the other for consistency. That's one of the primary jobs of an opinionated code formatter: consistency.

As per Dawn, well... priorities. New themes are built with the prettier plugin, but the plugin didn't exist when Dawn was written. There's only incremental value of adding it everywhere now in Dawn vs doing anything else given that it's mostly "done." New features are run with it, old ones may or may not be revisited.

While it's true that Liquid has been heavily influenced by Ruby, Liquid themes are made for the front-end. The Venn diagram of Ruby & Liquid developers is much smaller than the Venn diagram of JavaScript and Liquid developers. Having to remember which one to use is unnecessary. You can write nil, we'll format to null and call it a day.