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

chore: adds import/order eslint rule to demo-store template #895

Merged
merged 1 commit into from
May 17, 2023

Conversation

QuintonC
Copy link
Contributor

@QuintonC QuintonC commented May 16, 2023

WHY are these changes introduced?

For the demo-store:

  • The current linting configuration deviates from Remix in that it does not utilize the @typescript-eslint/consistent-type-imports rule.
  • The @remix-run/eslint-config package is installed in package.json, but was not being used in the eslint configuration.

Regarding import/order additions:

  • import/order is considered a Shopify best practice and is introduced here to reiterate and lean on that. shopify/web-configs/esnext

WHAT is this pull request doing?

This pull request adds import/order eslint rules for the demo-store as a start. Instead of adding it for the entire repository in one pull request, it makes the most sense to import this in smaller chunks.

To address the lint violations, a simple npm run lint --fix was used. There were only three additional issues which remained:

➜  demo-store git:(chore/demo-store-lint-configuration) ✗ npm run lint
npm WARN ignoring workspace config at /Users/quintonchester/Repos/hydrogen/templates/demo-store/.npmrc

> [email protected] lint
> eslint --no-error-on-unmatched-pattern --ext .js,.ts,.jsx,.tsx .

/Users/quintonchester/Repos/hydrogen/templates/demo-store/app/lib/utils.ts
  3:1  error  There should be no empty line within import group  import/order

/Users/quintonchester/Repos/hydrogen/templates/demo-store/app/routes/($locale).account.tsx
  133:10  error  'Account' is already defined  @typescript-eslint/no-redeclare

/Users/quintonchester/Repos/hydrogen/templates/demo-store/app/routes/($locale).products.$productHandle.tsx
  114:25  error  'Product' is already defined  @typescript-eslint/no-redeclare

✖ 3 problems (3 errors, 0 warnings)

Addressing those violations saw the following changes:

  • utils.ts 3:1
    • Removed the empty line within the import group.
  • ($locale).account.tsx
    • interface Account was updated to interface AccountType. Casted types were updated to reflect this change.
  • ($locale).products.$productHandle.tsx
    • The additional Product definition was an imported module which was seemingly unused -- please do check me on this as I was unable to determine the relevance of the import seen here. Diff

HOW to test your changes?

  • This should be tested primarily via green CI ✅
  • Do a test with the demo-store template from this branch.

Post-merge steps

Checklist

  • I've read the Contributing Guidelines
  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've added a changeset if this PR contains user-facing or noteworthy changes
  • I've added tests to cover my changes
  • I've added or updated the documentation

@@ -10,4 +10,38 @@ module.exports = {
'no-useless-escape': 'off',
'no-case-declarations': 'off',
},
overrides: [
{
files: ['./templates/demo-store/*'],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was included to ensure that the pre-commit scripts for eslint used the correct import/order definition since the lint configuration at the root of the monorepo will be used for the eslint check.

@QuintonC QuintonC force-pushed the chore/demo-store-lint-configuration branch from 7dcf57e to aa96bc6 Compare May 16, 2023 23:34
@@ -0,0 +1 @@
declare module 'typographic-base';
Copy link
Contributor Author

@QuintonC QuintonC May 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This module has no types, but was somehow passing the typecheck CI step previously. Not entirely sure how that was possible, but this resolves that issue 👍

Here is the failed CI run for those curious

@QuintonC QuintonC self-assigned this May 16, 2023
@QuintonC QuintonC marked this pull request as ready for review May 16, 2023 23:38
@benjaminsehl
Copy link
Member

This is awesome, thanks @QuintonC.

Does import order impact the Skeleton template? I recall your suggested order matching this.

@QuintonC
Copy link
Contributor Author

This is awesome, thanks @QuintonC.

Does import order impact the Skeleton template? I recall your suggested order matching this.

Not just yet -- I'm planning on addressing other templates after this one. I wanted to tackle them one at a time to prevent from having an extremely large pull request 😅

Copy link
Contributor

@frehner frehner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

One thing before we merge - can you add a patch changeset for the demo-store? We're keeping track of changes to the demo-store and using changesets to do it. Thanks!

@QuintonC
Copy link
Contributor Author

Looks great, thanks!

One thing before we merge - can you add a patch changeset for the demo-store? We're keeping track of changes to the demo-store and using changesets to do it. Thanks!

Absolutely. Thanks!

@QuintonC QuintonC force-pushed the chore/demo-store-lint-configuration branch from aa96bc6 to 2dcbaa3 Compare May 17, 2023 17:11
@QuintonC QuintonC merged commit 8ed46dc into 2023-04 May 17, 2023
@QuintonC QuintonC deleted the chore/demo-store-lint-configuration branch May 17, 2023 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants