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

style(eslint): enable @typescript-eslint/consistent-type-imports #9082

Conversation

virtuoushub
Copy link
Collaborator

@virtuoushub virtuoushub commented Aug 29, 2023

Enable a mostly stylistic lint rule. Will help with ESM conversion by helping differentiate type imports vs regular. This difference helps with false positives for the import/no-extraneous-dependencies rule ( #9081 ). That rule is what ultimately helps surface possible ESM issues/bundle bloat.

see also:


note: due to the broad nature of these changes, this can impact inflight PRs. hopefully any possible conflicts that arise are easy enough to fix via a "take your PRs changes" and then run yarn lint:fix merge strategy. Happy to help resolve any issues this PR might introduce to other inflight PRs, if it does get accepted/merged.

```
/code/redwood/packages/vite/src/fully-react/find-styles.ts
   6:9   error  `import()` type annotations are forbidden  @typescript-eslint/consistent-type-imports
   7:9   error  `import()` type annotations are forbidden  @typescript-eslint/consistent-type-imports
   8:13  error  `import()` type annotations are forbidden  @typescript-eslint/consistent-type-imports
  14:28  error  `import()` type annotations are forbidden  @typescript-eslint/consistent-type-imports
```
deps: Set<import('vite').ModuleNode>
vite: ViteDevServer,
node: ModuleNode,
deps: Set<ModuleNode>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Tobbe is there a reason to use import('vite'). here in this file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

gentle bump to @Tobbe - this is a fairly broad PR so the longer it stays open, the more painful the merge conflicts can become.

Copy link
Member

Choose a reason for hiding this comment

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

This is code I mostly copy/pasted from another project. Looks like import type { ModuleNode } from 'vite' should take care of the type here. Your change looks good

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks for double checking!

@virtuoushub
Copy link
Collaborator Author

@jtoar

This PR changes the create-redwood-app TS template.
That usually means the JS template needs to be rebuilt.
If you know that it doesn't, add the "crwa-ok" label. Otherwise, rebuild the JS template and commit the changes:

cd packages/create-redwood-app
yarn ts-to-js

pretty sure these changes do not require any JS template rebuild. I followed the above instructions locally, and no changes were produced.

@jtoar
Copy link
Contributor

jtoar commented Aug 29, 2023

@virtuoushub can I resolve the merge conflicts for you here?

This reverts commit 44f8dd2.
@virtuoushub
Copy link
Collaborator Author

virtuoushub commented Aug 29, 2023

@virtuoushub can I resolve the merge conflicts for you here?

Hi @jtoar , thanks however I already did. What is odd is it looks like yarn lint:fix is currently broken: #8572 (comment) I had not built the latest after pulling in #8572 which is why I was seeing that error.

@jtoar jtoar added this to the next-release milestone Aug 29, 2023
@jtoar jtoar added release:chore This PR is a chore (means nothing for users) crwa-ok Override the CRWA check labels Aug 29, 2023
ran `yarn rebuild-test-project-fixture`
Copy link
Contributor

@jtoar jtoar left a comment

Choose a reason for hiding this comment

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

Hey @virtuoushub, scanned these changes as best I could. Awesome work! I love where you're headed—I can't wait for the import/no-extraneous-dependencies rule. 🙏

I caught a few changes to the test project fixture that should be reverted. This is my miss, I need to update the script that rebuilds the test project fixture. We want to keep those stories the way they are. I commented on the files

Copy link
Contributor

@jtoar jtoar left a comment

Choose a reason for hiding this comment

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

LGTM pending the test project fixture changes and the question for @Tobbe.

@virtuoushub
Copy link
Collaborator Author

LGTM pending the test project fixture changes and the question for @Tobbe.

test project fixture changes are in.

…int/@typescript-eslint/consistent-type-imports

# Conflicts:
#	packages/web/src/components/GraphQLHooksProvider.tsx
#	packages/web/src/components/createCell.tsx
@virtuoushub
Copy link
Collaborator Author

fwiw it looks like there is an unrelated to these changes flakey test:

FAIL   code  src/__tests__/router.test.tsx (50.835 s)
  ● slow imports › usePageLoadingContext

    expect(element).not.toBeInTheDocument()

    expected document not to contain element, found <p>loading in page...</p> instead

      536 |       // This shouldn't show up, because the page shouldn't render before it's
      537 |       // fully loaded
    > 538 |       expect(screen.queryByText('loading in page...')).not.toBeInTheDocument()
          |                                                            ^
      539 |
      540 |       await waitFor(() => screen.getByText('done loading in page'))
      541 |       await waitFor(() => screen.getByText('done loading in layout'))

      at Object.toBeInTheDocument (src/__tests__/router.test.tsx:538:60)

see: https://github.com/redwoodjs/redwood/actions/runs/6032779642/job/16368480189#step:10:342

Copy link
Member

@Tobbe Tobbe left a comment

Choose a reason for hiding this comment

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

LGTM (only scanned the code, didn't try it)

@jtoar jtoar modified the milestones: next-release, v6.2.0 Sep 2, 2023
@Tobbe Tobbe merged commit 2897cd7 into redwoodjs:main Sep 5, 2023
29 checks passed
@virtuoushub virtuoushub deleted the virtuoushub/esm/eslint/@typescript-eslint/consistent-type-imports branch September 5, 2023 16:51
jtoar pushed a commit that referenced this pull request Sep 6, 2023
…9082)

Enable a mostly stylistic lint rule. Will help with ESM conversion by
helping differentiate `type` imports vs regular. This difference helps
with false positives for the `import/no-extraneous-dependencies` rule (
#9081 ). That rule is what
ultimately helps surface possible ESM issues/bundle bloat.

see also:
* https://gist.github.com/Jaid/164668c0151ae09d2bc81be78a203dd5
* https://stackoverflow.com/a/64243357

---

_note:_ due to the broad nature of these changes, this can impact
inflight PRs. hopefully any possible conflicts that arise are easy
enough to fix via a "take _your_ PRs changes" and then run `yarn
lint:fix` merge strategy. Happy to help resolve any issues this PR might
introduce to other inflight PRs, if it does get accepted/merged.
@jtoar jtoar modified the milestones: next-release, v6.2.0 Sep 6, 2023
@virtuoushub virtuoushub mentioned this pull request Dec 13, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crwa-ok Override the CRWA check release:chore This PR is a chore (means nothing for users)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants