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

feat(TypeScript): Update types #3739

Merged
merged 2 commits into from Apr 16, 2019

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Mar 29, 2019

review?(@crutchcorn, @Elchi3, @vinyldarkscratch): This no longer depends on #3737 and can be merged as‑is.

if (prop === 'support') {
checkVersions(data[prop]);
if (prop === '__compat' && data[prop].support) {
checkVersions(data[prop].support);
Copy link
Contributor Author

@ExE-Boss ExE-Boss Mar 29, 2019

Choose a reason for hiding this comment

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

This operation was previously type unsafe, and could lead to a potential crash if there was a feature in BCD named support.

@Elchi3 Elchi3 added infra 🏗️ Infrastructure issues (npm, GitHub Actions, releases) of this project not ready ⛔ This is not yet ready to be merged. It's pending a decision, other PR, or a prerequisite action. labels Mar 30, 2019
@ExE-Boss ExE-Boss force-pushed the typescript/add-matches-block branch 2 times, most recently from 1a962cf to 38364fa Compare April 9, 2019 11:59
@ExE-Boss
Copy link
Contributor Author

review?(@vinyldarkscratch): This no longer depends on #3737, so it can be merged as‑is.

P.S.: Can you please remove the not ready label?

@queengooborg queengooborg removed the not ready ⛔ This is not yet ready to be merged. It's pending a decision, other PR, or a prerequisite action. label Apr 13, 2019
@queengooborg queengooborg self-requested a review April 13, 2019 18:10
@@ -1,8 +1,6 @@
/* Any copyright is dedicated to the Public Domain.
* http://creativecommons.org/publicdomain/zero/1.0/ */

export as namespace bcd;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I previously misunderstood what export as namespace meant.

I now know that this means export as a UMD global, which doesn’t exist.

@ExE-Boss ExE-Boss force-pushed the typescript/add-matches-block branch from ac60de7 to 726016c Compare April 15, 2019 13:19
@queengooborg
Copy link
Collaborator

I'm not entirely certain what problem this fixes (and to be honest, I'm not experienced with TypeScript). From the basic tests to make sure the data still works as expected, I've found no issues. Is there a specific issue this resolves, or is it more of a maintenance update?

@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Apr 16, 2019

This is more of a maintenance update so that the types.d.ts file reflects the schema changes from #3631.

Copy link
Collaborator

@queengooborg queengooborg 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 answering my question, looking great!

@queengooborg queengooborg merged commit 509674d into mdn:master Apr 16, 2019
@ExE-Boss ExE-Boss deleted the typescript/add-matches-block branch April 16, 2019 17:15
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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants