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: fix ide complaints #2122

Merged
merged 2 commits into from
Sep 8, 2023
Merged

chore: fix ide complaints #2122

merged 2 commits into from
Sep 8, 2023

Conversation

tmkx
Copy link
Contributor

@tmkx tmkx commented Sep 7, 2023

Related Issues or Discussions

  1. Missing file extension "cjs" for "vitest/config"
  2. Option 'allowImportingTsExtensions' can only be used when either 'noEmit' or 'emitDeclarationOnly' is set

Summary

Check List

  • yarn run prettier for formatting code and docs

@vercel
Copy link

vercel bot commented Sep 7, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
jotai ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 7, 2023 2:46pm

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 7, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 38a6702:

Sandbox Source
React Configuration
React Typescript Configuration
React Browserify Configuration
React Snowpack Configuration
Next.js Configuration
Next.js with custom Babel config Configuration
React with custom Babel config Configuration

Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

Thanks for the suggestion.

We should do the same in zustand and valtio too.

Meanwhile, please see these minor comments.

.eslintrc.json Outdated Show resolved Hide resolved
@@ -11,6 +11,7 @@
"exactOptionalPropertyTypes": true,
"baseUrl": ".",
"skipLibCheck": true,
"noEmit": true,
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this even if we have --noEmit option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not anymore

Copy link
Contributor Author

@tmkx tmkx Sep 7, 2023

Choose a reason for hiding this comment

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

sorry i misunderstood, yes we need it because allowImportingTsExtensions is only allowed when --noEmit or --emitDeclarationOnly is enabled, since these import paths would not be resolvable at runtime in JavaScript output files.

https://www.typescriptlang.org/tsconfig#allowImportingTsExtensions

Copy link
Member

Choose a reason for hiding this comment

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

We used to have --noEmit in package.json and no "noEmit" in tsconfig.json. Does it cause any errors with this PR's change?
I mean I'm aware of https://www.typescriptlang.org/tsconfig#allowImportingTsExtensions and it's working before this PR, so I'm confused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it'ok, we are bundling with rollup instead of tsc.
if you remove the --noEmit option in the pretest script, you will be able to see the complaint in the console.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I know. I'm fine with having "noEmit" in tsconfig.json, but in that case remove --noEmit from pretest script to avoid duplication. But, if --noEmit works without adding "noEmit" in tsconfig.json, it keep the original idea, so it seems better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

VSCode(tsserver) will show an error in the tsconfig.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Member

Choose a reason for hiding this comment

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

Alright! thanks for the clarification!

Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

The change looks good.
Can you also open the same PR in the two repos? https://github.com/pmndrs/zustand and https://github.com/pmndrs/valtio

@tmkx
Copy link
Contributor Author

tmkx commented Sep 7, 2023

there is a flaky test case :(

@dai-shi
Copy link
Member

dai-shi commented Sep 7, 2023

there is a flaky test case :(

Yeah... Maybe using user-event would help.

@dai-shi
Copy link
Member

dai-shi commented Sep 7, 2023

Yeah... Maybe using user-event would help.

Hmm, it('does not show async stale result', async () => { is already using userEvent.

Feel free to dig into it!
https://github.com/pmndrs/jotai/actions/runs/6111275676/job/16586153332

@dai-shi dai-shi merged commit 2526039 into pmndrs:main Sep 8, 2023
@tmkx tmkx deleted the fix/ide-complain branch September 13, 2023 02:54
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.

2 participants