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

Add test-types script back #1622

Merged
merged 2 commits into from
Sep 12, 2021
Merged

Add test-types script back #1622

merged 2 commits into from
Sep 12, 2021

Conversation

jablko
Copy link
Contributor

@jablko jablko commented Aug 28, 2021

I ran into an issue with test-types which this PR solves by adding "types": [] -> **/tsconfig.json. If it was the issue with test-types, perhaps it can be restored -> the CI?

#1179 temporarily moved test-types -> its own types.yml workflow, suggesting it be moved back -> test once it's running stably:

This will be a temporary change until we see dtslint running stably, then we can move test-types back under the test command.

types.yml was subsequently eliminated in #1338.

The issue I encountered was dtslint implicitly including and checking every node_modules/@types declaration:

Error: Errors in [email protected] for external dependencies:
../../../node_modules/@types/prettier/index.d.ts(537,14): error TS2456: Type alias 'Doc' circularly references itself.

    at /home/nottheoilrig/mdx/node_modules/dtslint/bin/index.js:206:19
    at Generator.next (<anonymous>)
    at fulfilled (/home/nottheoilrig/mdx/node_modules/dtslint/bin/index.js:6:58)

This PR solves that by adding "types": [] -> **/tsconfig.json:

$ for tsconfig in $(git ls-files '**/tsconfig.json'); do
  git show ":$tsconfig" | jq '.compilerOptions.types = []' > "$tsconfig"
done

This will still type check our explicit node_modules/@types dependencies, however it will no longer implicitly include every node_modules/@types declaration.

@vercel
Copy link

vercel bot commented Aug 28, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/mdx/mdx/7hs49bFavYZ2hSVmppEU6waYoESw
✅ Preview: https://mdx-git-fork-jablko-patch-4-mdx.vercel.app

@vercel vercel bot temporarily deployed to Preview August 28, 2021 19:23 Inactive
@vercel vercel bot temporarily deployed to Preview August 28, 2021 20:09 Inactive
@vercel vercel bot temporarily deployed to Preview August 28, 2021 21:04 Inactive
@@ -28,8 +28,8 @@
"publish-next": "lerna publish --force-publish=\"*\" --pre-dist-tag next --preid next",
"test-api": "lerna run test-api",
"test-coverage": "lerna run test-coverage",
"test-types": "lerna run test-types",
"test": "yarn build && yarn lint && yarn test-coverage"
"test-types": "lerna --concurrency 1 run test-types",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Concurrent dtslint invocations were trampling each other installing TypeScript versions ... At least I think that's what was happening here -- and here ... Perhaps here too ...

Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

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

Sounds alright to me.
Thoughts on this approach @JounQin?

@ChristianMurphy ChristianMurphy added ☂️ area/types This affects typings 🕸️ area/tests This affects tests labels Aug 29, 2021
@JounQin
Copy link
Member

JounQin commented Aug 30, 2021

I'm not sure, personally I'd prefer something like Expect type utility for types.test.ts, and run tsc --noEmit for type checking for all .ts related files.

But for multiple ts versions, dtslint maybe better.

However, I don't think targeting multiple ts versions are worth unless we're not going to always follow latest TypeScript.

@jablko
Copy link
Contributor Author

jablko commented Sep 10, 2021

@JounQin I think most of this change applies equally to tsc --noEmit and dtslint: Without "types": [] we'll implicitly include every node_modules/@types declaration in either case, I think?

Can we land this, and roll back the lerna --concurrency 1 option if/when we drop dtslint?

Copy link
Member

@JounQin JounQin left a comment

Choose a reason for hiding this comment

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

OK, whether to drop dtslint can be discussed later, let's fix current issues first.

@wooorm wooorm changed the title Restore test-types -> CI Add test-types script back Sep 12, 2021
@wooorm wooorm merged commit 6a27dbd into mdx-js:main Sep 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🕸️ area/tests This affects tests ☂️ area/types This affects typings
Development

Successfully merging this pull request may close these issues.

4 participants