-
Notifications
You must be signed in to change notification settings - Fork 350
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
Document perseus-types.ts file #1975
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@khanacademy/perseus": patch | ||
--- | ||
|
||
Make all types in `perseus-types.ts` originate from it (no longer import Mafs types) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,40 @@ | ||
// TODO(FEI-4010): Remove `Perseus` prefix for all types here | ||
// TODO(FEI-4011): Use types generated by https://github.com/jaredly/generate-perseus-flowtypes | ||
/** | ||
* The Perseus "data schema" file. | ||
* | ||
* This file, and the types in it, represents the "data schema" that Perseus | ||
* uses. The @khanacademy/perseus-editor package edits and produces objects | ||
* that conform to the types in this file. Similarly, the top-level renderers | ||
* in @khanacademy/perseus, consume objects that conform to these types. | ||
* | ||
* WARNING: This file should not import any types from elsewhere so that it is | ||
* easy to reason about changes that alter the Perseus schema. This helps | ||
* ensure that it is not changed accidentally when upgrading a dependant | ||
* package or other part of Perseus code. Note that TypeScript does type | ||
* checking via something called "structural typing". This means that as long | ||
* as the shape of a type matches, the name it goes by doesn't matter. As a | ||
* result, a `Coord` type that looks like this `[x: number, y: number]` is | ||
* _identical_, in TypeScript's eyes, to this `Vector2` type `[x: number, y: | ||
* number]`. Also, with tuples, the labels for each entry is ignored, so `[x: | ||
* number, y: number]` is compatible with `[min: number, max: number]`. The | ||
* labels are for humans, not TypeScript. :) | ||
* | ||
* If you make changes to types in this file, be very sure that: | ||
* | ||
* a) the changes are backwards compatible. If they are not, old data from | ||
* previous versions of the "schema" could become unrenderable, or worse, | ||
* introduce hard-to-diagnose bugs. | ||
* b) the parsing code (`util/parse-perseus-json/`) is updated to handle | ||
* the new format _as well as_ the old format. | ||
*/ | ||
|
||
import type {Coord} from "./interactive2/types"; | ||
import type {Interval, vec} from "mafs"; | ||
// TODO(FEI-4010): Remove `Perseus` prefix for all types here | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we even still want to remove the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe we still could, yes. Today, some of these type names get quite wordy. But, I don't think we need to deal with that now. |
||
// Range is replaced within this file with Interval, but it is used elsewhere | ||
// and exported from the package, so we need to keep it around. | ||
export type Coord = [x: number, y: number]; | ||
export type Interval = [min: number, max: number]; | ||
export type Vector2 = Coord; // Same name as Mafs | ||
export type Range = Interval; | ||
export type Size = [number, number]; | ||
export type CollinearTuple = [vec.Vector2, vec.Vector2]; | ||
export type Size = [width: number, height: number]; | ||
export type CollinearTuple = [Vector2, Vector2]; | ||
Comment on lines
-5
to
+36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These changes are to "internalize" all types into this file instead of importing them from "outside". This means that if we upgrade Mafs and a type changes, we won't be implicitly breaking the schema. We'd still have type mismatches here in Perseus, but we'd then be able to rationalize the differences between our schema (this file) and the changed types. |
||
export type ShowSolutions = "all" | "selected" | "none"; | ||
|
||
/** | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only feedback is that this seems to confident. It does represent all permutations of the data or it should?
But if you feel confident then just 🚢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's aspirational. :) The new parsing code validates against these types and so I think it's a fair statement.