[POC][WIP] Hydrogen eslint plugin

[cartogram]

Hydrogen ESLint Plugin Proposal

• GitHub Discussion

Overview

This RFC exists to propose a new ESLint setup for Hydrogen, both as a solution to our monorepo used in development and also for users building Hydrogen storefronts.

Background

In version 1 of hydrogen we had an eslint-plugin package that served to provide the following:

1. Common Configurations

These level-set on common ESLint rules to enforce Shopify’s JavaScript best practices, and catch common issues when using Hydrogen. We maintain these so merchants don't need to worry about the undifferentiated setup common to every Hydrogen app. ESLint is a the standard tool for JavaScript and TypeScript, and providing either a base config or plugin is common among almost every other (meta-)framework.

We provide 3 possible configurations:

• The recommended configuration enforces Shopify's JavaScript best practices and includes third-party plugins.

{
  extends: 'plugin:hydrogen/recommended',
}

The Hydrogen configuration excludes suggested third-party plugins, but keeps the custom Hydrogen rules with their suggested defaults.

{
  extends: 'plugin:hydrogen/hydrogen',
}

The TypeScript configuration is a partial set of overrides to augment the recommended or Hydrogen configurations.

{
  extends: ['plugin:hydrogen/recommended', 'plugin:hydrogen/typescript'],
}

2. Custom Rules

We also provided a set of custom rules that only made sense in projects using Hydrogen v1.

• server-component-banned-hooks: Prevents using the useState, useReducer, useEffect, and useLayoutEffect hooks in files that don't end with the .client extension
• client-component-banned-hooks: Prevents using the useQuery hook in files that end with the .client extension
• prefer-image-component: Prevents using the img tag directly and suggests using the Image component from @shopify/hydrogen
• server-no-json-parse: Prevents using JSON.parse in a Hydrogen API route or server component
• prefer-gql: Detects the use of a GraphQL utility other than the one provided by Hydrogen

Proposal

This proposal recommends the configurations and custom rules will move into the new H2 repo as a new eslint-plugin package. The details of this migration are outlined below.

New configurations

• We will export multiple configurations to enable this package to be used for both storefront developers and contributors. These configurations will be divided into the following domains:
  • storefront: This configuration will be exposed for Hydrogen storefronts and used across our demo-store and hello-world templates and any future scaffolding commands that relate to eslint (for example, h2 setup eslint). It will contain a combined config of the internal configs for react, remix and hydrogen.
  • workspace: This configuration will be used in the mono-repo root and used across our packages.
  • cli: This configuration will be used in the hydrogen packages cli-hydrogen and create-hydrogen specifically. It will contain a the configurations and custom rules from the @shopify/eslint-plugin-cli.
  • library: This configuration will be used in the hydrogen-react package. It is more strict when it comes to typescript.

New rules

• We will remove all custom rules except prefer-image-component, as it's the one that makes sense in the H2 context.

• no-missing-error-boundary-in-route: Have an ErrorBoundary in every route template. ErrorBoundary is used when an Error is thrown in a “loader”, and is generally meant for unexpected errors, like 500, 503, etc. Any Storefront query or mutation error will be handled by the ErrorBoundary. Type the error as “unknown” since anything in JS can be thrown 🙂

• require-error-element-on-await: Use the “errorElement” prop on every <Await> component. When using “defer”, some promises may be rejected at a later time. The only way to handle this is to use the “errorElement” on the associated component, otherwise the error is swallowed.

• prefer-try-catch-in-route Use try/catch – except in “loader”, “action”, and the Component. Those three “Route Module APIs” are handled automatically by ErrorBoundary and CatchBoundary, but the rest – such as “meta”, “links”, “handle”, etc. – will crash the server if an error is thrown.

• no-try-catch-in-loader and no-try-catch-in-action: For templates it’s easier to let the error be thrown and get handled by the ErrorBoundary than to handle it manually.

• route-module-export-order: Remix-specific route API functions should be ordered and consistent in style, to help developers quickly scan and find what they're looking for. Order these APIs following a top-down order of concerns:

  1. Http header tweaks (shouldRevalidate, headers, meta, links)
  2. Data manipulation (loader, action)
  3. UI (Component)
  4. Error handling (ErrorBoundary, CatchBoundary)
  5. Storefront API GraphQL query strings

• enforce-server-only - checks if imported functions are only called inside loaders or actions, and if so recommend the module to be re-named with .server.

• prevent-returning-private-env-vars - This could check that loaders don't return PRIVATE_ prefixed env variables, to avoid exposing secrets.

Detailed design

Tech stack

The tech stack of the new plugin will be consistent with all hydrogen packages, written in typescript and tested with vitest.

How to write rules

Rules are structured as follows:

• /README.md: contains all documentation and usage instructions.
• /<name-of-rule>.ts: contains all code required to evaluate the rule.
• /<name-of-rule>.test.ts: contains a suite of tests to validate the rule's valid and invalid states.
• /index.ts: re-exports the rule to the outer world.

Rules should auto-fix when possible and contain clear/ consistent messaging that directs users on where they can learn more.

Incremental adoption

Rather than publish this new package overtop of the old library on NPM, I suggest we release it as eslint-plugin-h2. Overtime we can deprecate, remove and overwrite eslint-plugin-hydrogen when we are confident with the developer experience.

Future proof

Rules are easy to write and maintain and we will be constantly evolving this library as Remix and other Hydrogen dependents change.

Alternate ideas

Do nothing

We've haven't received negetive feedback about ESLint, nor have we been burdened by the fact the old eslint-plugin is in the old Hydrogen v1 repo and not easily patchable. So why do anything and why do it now?

1. Reduce risk: Though we haven't received any problems with the library, it presents a huge risk for future toil. We have an opportunity to be reactive here without much upfront cost.
2. Capitalize on DX: ESLint is widely used and built into every IDE in someway. Let's use this to create custom rules that guide merchant developers into the pit of success.
3. External integrations: Given more predictability in the ESLint setup of Hydrogen storefronts we can more easily interact with the code via CLI, the admin channel and whatever else comes in the future.
4. Refactor monorepo setup: Doing nothing also leaves us in a cobbled-together setup for ESLint. This proposal takes the monorepo into account, moving it to a more maintainable configuration.

[wizardlyhel]

I am in favour of setting up eslint plugin.

Thoughts on the new rules:

• no-missing-error-boundary-in-route - Should eslint enforce error boundary be defined on every route? If it is not defined, it should fallback to the error boundary defined by the parent route. I do think we should enforce error boundary define in root.tsx but I don't think it should be required for every routes.
• route-module-export-order - This one is a code of conduct that we Hydrogen team set for ourselves. Does it make sense to also ask developers do the same? Should we default it to off for those extending our eslint plugin?

[frehner]

For context, there have been a couple of discussions I've had in discord about the prefer-image-component eslint rule (which also implies that there are a couple of devs out there actually using our eslint rules 🙂 ).

I'm not sure it makes sense to enforce using the <Image/> component in v2; while it's nice that the <Image/> component can be hooked up to load images from other places, it's not its primary purpose and it's a tiny bit painful to do so (at least, that was my understanding! I could be wrong here).

I think it's plenty valid for devs to use a normal <img> tag for images that are from the local file system or things like that.

---

We will export multiple configurations to enable this package to be used for both storefront developers and contributors. These configurations will be divided into the following domains:
storefront: ...
workspace: ...
cli: ...
library: ...

Do you have examples of the rules for each? In looking at the New Rules section, it seems like everything (for now?) would go into storefront, right?

[wizardlyhel]

Oh! @frehner you have a good point about the image rule. It would become an annoyance to the developer they they just want to have an image with <img> tag. For example, using <img> for tracking pixel or use it to show a base64 image source.

But I think the rule would be fine if it is just an enforcement of our own components like .. <CartProvider> vs <CartProviderV2>

[lordofthecactus]

👍 I'm positive about having an eslint plugin and finding rules we can add to

On publishing:

Rather than publish this new package overtop of the old library on NPM, I suggest we release it as eslint-plugin-h2. Overtime we can deprecate, remove and overwrite eslint-plugin-hydrogen when we are confident with the developer experience.

We can't remove or overwrite eslint-plugin-hydrogen package unfortunately, although we can mark the releases as deprecated if we wanted.

I would keep eslint-plugin-h2 (or eslint-plugin-hydrogen-remix) and just keep whichever you choose.

Let me know if there is a particular feedback you are looking for.

On the rules, agree with the comments from @wizardlyhel and @frehner, maybe we could dogfood eslint rules within our repo, and slowly release as we find they are useful?

[cartogram]

I'm not sure it makes sense to enforce using the component in v2; while it's nice that the component can be hooked up to load images from other places, it's not its primary purpose and it's a tiny bit painful to do so (at least, that was my understanding! I could be wrong here)

Yeah, I too understand the frustration with this rule and would like to improve upon that. In this case, it looks to me like the better solution would be to make the Image component easier to work with in all cases? @wizardlyhel think the tracking pixel is a random exception, so they would probably disable the rule for that one.

Do you have examples of the rules for each? In looking at the New Rules section, it seems like everything (for now?) would go into storefront, right?

Yeah that's right @frehner! Internally Internally we might break the remix-specific ones to a separate config, but for users this would all get bundled up into storefront. I don't see us writing custom rules for our development purposes too often. The CLI team does that for the CLI, however.

route-module-export-order - This one is a code of conduct that we Hydrogen team set for ourselves. Does it make sense to also ask developers do the same? Should we default it to off for those extending our eslint plugin?

I just added a quick stab at this rule and it caught 5 violations in the demo-store. I think we can default to off or warn, but enable it in our templates. If a user finds it too strict they can disable it from there quite easily.

Should eslint enforce error boundary be defined on every route?

Good question. In this case I was going off the of the template reccomendations. As I think about it more, we could say that if there is a loader or action defined, then we enforce an error boundary. @wizardlyhel what do you think?

[cartogram]

@lordofthecactus your comment came in just as I was responding to the others.

I'm positive about having an eslint plugin and finding rules we can add to

:)

I would keep eslint-plugin-h2 (or eslint-plugin-hydrogen-remix) and just keep whichever you choose.

I am for the shorter option, but open to what others think. Do you have a pref?

Let me know if there is a particular feedback you are looking for.

I think the main things for reviewers is to read the proposal, but addtionally:

• Do the rules look useful? Is there anything I am missing?
• Does separation of configs make sense?
• Examine each config and let me know if the disabled/enabled rules are appropriate for the use-cases.

Optionally if you would like to help get this over the line, let me know (not mandatory). You can also run this POC to see the effects running eslint has across the packages/templates if that makes it easier to follow.

[cartogram]

I updated the list of rules to include a few provided by @juanpprieto IRL.

enforce-server-only - checks if imported functions are only called inside loaders or actions, and if so recommend the module to be re-named with .server.
prevent-returning-private-env-vars - This could check that loaders don't return PRIVATE_ prefixed env variables, to avoid exposing secrets.

[cartogram]

Do you have examples of the rules for each?

@frehner Yeah, I'm still working through this, but you can see the different configs (and how they relate/extend from one another) in the changed files area in the packages/eslint-plugin/src/configs directory. Would love feedback on this aspect if you get a chance to take a look. I think I want to delineate the ones that are exposed (storefront, cli...) vs the ones that are shared (core, react...).

[cartogram]

I am considering adding an oxygen specific config and ruleset. The first rule would be:

• oxygen/no-unsupported-features: Inform the merchant developer across all relevant surfaces when they are using runtime-specific APIs such as HtmlRewriter and WebSockets. (cc @lancelafontaine @maxshirshin)

[maxshirshin]

I am considering adding an oxygen specific config and ruleset. The first rule would be:

• oxygen/no-unsupported-features

That's a great idea (and something we really need, as we aren't enforcing this in any other way otherwise). Should we probably call it oxygen/api-compatibility or smth similar, so that we don't focus on unsupported features but rather on anything that might be an issue in the future if we change the platform?

This would include the use of HTMLRewriter and WebSockets for now, which we didn't list on our docs as supported, but people still may try them because they exist in the CF runtime. I'm also sure we'll find a couple of platform-specific methods or properties on common classes that we'd like to warn about if they're used.

[michenly]

Close due to inactivity