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 unittest to validate TypeScript types #16590

Closed
saschanaz opened this issue Jun 7, 2022 · 5 comments · Fixed by #16591 or #16703
Closed

Add unittest to validate TypeScript types #16590

saschanaz opened this issue Jun 7, 2022 · 5 comments · Fixed by #16591 or #16703
Labels
infra 🏗️ Infrastructure issues (npm, GitHub Actions, releases) of this project

Comments

@saschanaz
Copy link
Contributor

For example, BrowserNames is now gone but the schema still refers to it, causing build failure:

"tsType": "Partial<Record<BrowserNames, SupportStatement>>"

There are also several occurrences of import('../../types').Identifier in linters which are now all broken:

* @typedef {import('../../types').Identifier} Identifier

@saschanaz
Copy link
Contributor Author

saschanaz commented Jun 7, 2022

https://github.com/mdn/browser-compat-data/releases/tag/v5.0.2

This release includes a fundamental change to the TypeScript definitions.

That sounds like a breaking change 🤔, there should be some deprecated typedefs until 6.x.

@queengooborg queengooborg added bug 🐛 Confirmed bugs in the repository (i.e. linter bugs), NOT browser implementation bugs. infra 🏗️ Infrastructure issues (npm, GitHub Actions, releases) of this project labels Jun 8, 2022
@queengooborg
Copy link
Collaborator

TypeScript definitions were not covered by semantic versioning during this release. However, going forward, they will be (especially since they're auto-generated). I've just opened up a fix for the typo, and all of those linter typedef imports will be fixed in #16333.

@saschanaz
Copy link
Contributor Author

That's not all. Please reopen and add tests before closing this.

node_modules/@mdn/browser-compat-data/types.d.ts:134:44 - error TS2344: Type 'Omit<string, "__compat">' does not satisfy the constraint 'string | number | symbol'.

134 export interface Identifier extends Record<Omit<string, '__compat'>, Identifier> {
                                               ~~~~~~~~~~~~~~~~~~~~~~~~

node_modules/@mdn/browser-compat-data/types.d.ts:255:3 - error TS2411: Property '__compat' of type 'CompatStatement | undefined' is not assignable to 'string' index type 'Identifier'.

255   __compat?: CompatStatement;

@queengooborg
Copy link
Collaborator

I closed this issue because any remaining TypeScript definition issues will be resolved in #16333, when BCD's internals are migrated to TypeScript and depend on these types. I plan to release an update to BCD later this week to deploy TypeScript fixes.

@queengooborg
Copy link
Collaborator

Ah, wait: I now see this is talking about writing a unittest. Apologies, I thought this was just about the invalid TypeScript definition.

For the unittest, if you're down to write a PR to add one (and fix up any issues in separate PRs), I'd be happy to review and merge it.

@queengooborg queengooborg reopened this Jun 8, 2022
@queengooborg queengooborg changed the title The generated types.d.ts is broken; Add tests for that Add unittest to validate TypeScript types Jun 8, 2022
@queengooborg queengooborg removed the bug 🐛 Confirmed bugs in the repository (i.e. linter bugs), NOT browser implementation bugs. label Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra 🏗️ Infrastructure issues (npm, GitHub Actions, releases) of this project
Projects
None yet
2 participants