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.

Sign up for GitHub

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

Jump to bottom

Stricter types #16699

Merged
merged 8 commits into from
Jun 17, 2022
Merged

Stricter types #16699

merged 8 commits into from
Jun 17, 2022

Conversation

bershanskiy
Copy link
Contributor

Summary

Refactor the type system a bit and remove extra type casts (thus making code more type-sound).

Test results and supporting details

This is pure refactoring.

Related issues

#16668

@github-actions github-actions bot added infra 🏗️ Infrastructure issues (npm, GitHub Actions, releases) of this project linter 🏡 Issues or pull requests regarding the tests / linter of the JSON files. schema ⚙️ Isses or pull requests regarding the JSON schema files used in this project. scripts 📜 Issues or pull requests regarding the scripts in scripts/. labels Jun 16, 2022
@github-actions github-actions bot added the bulk_update 📦 An update to a mass amount of data, or scripts/linters related to such changes label Jun 16, 2022
@bershanskiy bershanskiy mentioned this pull request Jun 16, 2022
Merged
queengooborg
queengooborg reviewed Jun 17, 2022
View reviewed changes
scripts/fix/browser-order.ts Outdated
@@ -23,15 +23,17 @@ export const orderSupportBlock = (
value: CompatStatement,
): CompatStatement => {
if (key === '__compat') {
const support = Object.keys(value.support)
const support: CompatStatement['support'] = (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't it be better to do this instead?

Suggested change
const support: CompatStatement['support'] = (
const support: SupportStatement = (

(Please feel free to ignore if this is just another one of those dumb TypeScript nits!)

@bershanskiy bershanskiy marked this pull request as draft June 17, 2022 06:08
@bershanskiy
Copy link
Contributor Author

I dropped changes to the following files:

  • schemas/compat-data.schema.json
  • test/linter/test-consistency.ts
  • types/index.d.ts

They were changing interfaces too much which is probably bad now considering the BCD export any cast and that we might want to replace "mirror" statement with lack or anything at all.

@bershanskiy bershanskiy marked this pull request as ready for review June 17, 2022 06:56
@queengooborg queengooborg merged commit 5b8ab8f into mdn:main Jun 17, 2022
@bershanskiy bershanskiy deleted the types branch June 17, 2022 11:10
@bershanskiy
Copy link
Contributor Author

@rebloor Which commit are you on? I'm on current main 2776fb5 and can not reproduce.

@rebloor rebloor mentioned this pull request Jun 17, 2022
Merged
@rebloor
Copy link
Collaborator

rebloor commented Jun 17, 2022

I pulled from Main a few minutes ago. Let me go and try main itself

@rebloor
Copy link
Collaborator

rebloor commented Jun 17, 2022

Sorry delete my earlier comment, was recommended to create a new issue

@rebloor
Copy link
Collaborator

rebloor commented Jun 17, 2022

I get the same issue off of Main, creating a new issue

@bershanskiy
Copy link
Contributor Author

For anyone reading this: that issue is #16716, let's continue the discussion there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@queengooborg queengooborg queengooborg approved these changes

Assignees
No one assigned
Labels
bulk_update 📦 An update to a mass amount of data, or scripts/linters related to such changes infra 🏗️ Infrastructure issues (npm, GitHub Actions, releases) of this project linter 🏡 Issues or pull requests regarding the tests / linter of the JSON files. schema ⚙️ Isses or pull requests regarding the JSON schema files used in this project. scripts 📜 Issues or pull requests regarding the scripts in scripts/.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bershanskiy @rebloor @queengooborg