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

ci(Testing): add TypeScript test job #4002

Merged
merged 2 commits into from Apr 4, 2020

Conversation

SpaceEEC
Copy link
Member

@SpaceEEC SpaceEEC commented Mar 27, 2020

Please describe the changes this PR makes and why it should be merged:

This PR adds a TypeScript job to the Testing and Testing Cron workflows to ensure that the typings do not include any errors making a successful build impossible.

Related issue: #3451
Not exactly resolving it, as my testing file is rather small and mostly there to have something to build, and not exactly to test whether all of the typings are working correctly.

An example of a breaking build: https://github.com/SpaceEEC/discord.js/actions/runs/64987237

In the tsconfig.json:

I replaced skipLibCheck with skipDefaultLibCheck to actually validate our index.d.ts file.
I changed pretty to false as that's the format the setup-node action's problem matcher expects.
(Although it still seems to not pick up the line numbers correctly, no clue if there is something I can do about that)

Status

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

Semantic versioning classification:

  • This PR changes the library's interface (methods or parameters added)
    • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)
  • This PR only includes non-code changes, like changes to documentation, README, etc.

@iCrawl
Copy link
Member

iCrawl commented Mar 28, 2020

(Although it still seems to not pick up the line numbers correctly, no clue if there is something I can do about that)

We can register our own matcher with a custom regex if that helps

@SpaceEEC
Copy link
Member Author

That indeed works, annotation are visible now: SpaceEEC@5ff16f8

Relevant changes I did to get it to work:
SpaceEEC@6057f57#diff-1ec633e3a9cc3ff453267d163598360b
SpaceEEC@6057f57#diff-fe8421955fd596131bb6f1b78984b2fbR55

This is a reported issue actions/setup-node#97 with a PR for this open already actions/setup-node#125.

Should I include those changes now and later remove them again once the above PR gets released or should I not and we just let it fix itself eventually?

@iCrawl
Copy link
Member

iCrawl commented Mar 28, 2020

Should I include those changes now and later remove them again once the above PR gets released or should I not and we just let it fix itself eventually?

Yes, former

I removed the skipLibCheck option to actually validate our index.d.ts file.

We should enable this again, because we shouldn't need to verify node typings for example (in case of a broken release, which means we can't release our software then), and just mark our typings file as non-default: https://www.typescriptlang.org/docs/handbook/triple-slash-directives.html#-reference-no-default-libtrue
Now that I properly read this, we shouldn't even be affected?

@SpaceEEC
Copy link
Member Author

Enabling skipLibCheck seems to always skip all lib checks without an option to exclude our own.

Using skipDefaultLibCheck does not really get us far either as this only excludes libs explicitly marked as default. (This option is apparently deprecated)

Since you mentioned it, the @types/node declarations are not marked as default:
https://github.com/didinele/DefinitelyTyped/blob/master/types/node/v12/fs.d.ts#L1

Default libraries are, for example:
https://github.com/microsoft/TypeScript/blob/master/lib/lib.d.ts#L18
https://github.com/microsoft/TypeScript/blob/master/lib/lib.es2017.object.d.ts#L18

I don't see a way to skip all libs except our own.

@iCrawl iCrawl merged commit 828640c into discordjs:master Apr 4, 2020
@SpaceEEC SpaceEEC deleted the ci/test_typescript_build branch April 11, 2020 07:02
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.

None yet

2 participants