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

Port domTree and buildCommon to @flow. #938

Merged
merged 7 commits into from Nov 24, 2017

Conversation

@marcianx
Copy link
Collaborator

commented Oct 15, 2017

Fixes #939.

@marcianx marcianx requested a review from kevinbarabash Oct 15, 2017

@marcianx marcianx force-pushed the marcianx:flow-domTree branch from 1f210d3 to dfeed9d Oct 26, 2017

@marcianx marcianx changed the title Port domTree to @flow. Port domTree and buildCommon to @flow. Oct 26, 2017

@marcianx marcianx force-pushed the marcianx:flow-domTree branch 2 times, most recently from f73bab5 to 6cbd82b Oct 26, 2017

@marcianx

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 26, 2017

This PR is now ready as well.

@@ -9,6 +10,12 @@ import fontMetrics from "./fontMetrics";
import symbols from "./symbols";
import utils from "./utils";

import type Options from "./Options";
import type ParseNode from "./ParseNode";

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Nov 11, 2017

Member

Shouldn't these also be wrapped in {} like the import type statements below?

This comment has been minimized.

Copy link
@marcianx

marcianx Nov 13, 2017

Author Collaborator

They are both exported via export default so that they need to be imported directly (i.e. without {}).

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Nov 24, 2017

Member

I didn't realize that you could import a class from another module as a type, cool.

const lookupSymbol = function(value, fontFamily, mode) {
const lookupSymbol = function(
value: string,
fontFamily: string,

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Nov 11, 2017

Member

It would be nice to better specific this. Could be left as a TODO though.

This comment has been minimized.

Copy link
@marcianx

marcianx Nov 13, 2017

Author Collaborator

Agreed. As this would touch more files, I'm deferred it to another PR (I opened #963 to track this). The font family is is sometimes a computed string:
https://github.com/Khan/KaTeX/blob/d969322dd2868776ebc10c71c459d56b502b1222/src/delimiter.js#L95
which will probably require a $FlowFixMe after an assertion for size range.

@@ -78,8 +96,15 @@ const makeSymbol = function(value, fontFamily, mode, options, classes) {
/**
* Makes a symbol in Main-Regular or AMS-Regular.
* Used for rel, bin, open, close, inner, and punct.
*
* TODO(#953): Make `options` mandatory and always pass it in.

This comment has been minimized.

Copy link
@kevinbarabash

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Nov 11, 2017

Member

We probably want to do this for makeSymbol as well.

This comment has been minimized.

Copy link
@marcianx

marcianx Nov 13, 2017

Author Collaborator

Done.


export interface CombinableDomNode extends VirtualDomNode {
tryCombine(sibling: CombinableDomNode): boolean;
}

This comment has been minimized.

Copy link
@kevinbarabash
let height = 0;
let depth = 0;
let maxFontSize = 0;

if (elem.children) {

This comment has been minimized.

Copy link
@kevinbarabash
const firstChild = params.children[0];
if (firstChild.type !== "elem") {
throw new Error('First child must have type "elem".');
}

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Nov 11, 2017

Member

This is can happen if firstChild.type === "kern"?

This comment has been minimized.

Copy link
@marcianx

marcianx Nov 13, 2017

Author Collaborator

firstChild.type can never be "kern". I couldn't enforce this at the type level except by doing one of the following approaches, both of which I found awkward.

  1. Change children: VListChild[] to be passed instead as firstChild: VListElem, restChildren: VListChild[], where the former must be elem. (This requires changing everywhere makeVList is called.)
  2. Change children: VListChild[] to a union of tuple types (and see if the compiler is happy with writing for loops over them) like
children: [VListElem] | [VListElem, VListChild] | [VListElem, VListChild, VListChild] | [VListElem, VListChild, VListChild, VListChild] | ...

up to the max number of children passed anywhere in our code base. This is a change only to this file, but I wasn't sure whether I'd have to cajole the compiler more when accepting some of the input or using children.

*
* See VListParam documentation above.
*/
const makeVList = function(params: VListParam, options: Options): domTree.span {

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Nov 11, 2017

Member

makeVList does a lot of stuff, thanks for breaking it up a bit.

@@ -423,7 +505,7 @@ const makeVerb = function(group, options) {

// A map of spacing functions to their attributes, like size and corresponding
// CSS class
const spacingFunctions = {
const spacingFunctions: {[string]: {| size: string, className: string |}} = {

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Nov 11, 2017

Member

I was confused for a second until I realize that [string] was the key.

This comment has been minimized.

Copy link
@marcianx

marcianx Nov 13, 2017

Author Collaborator

I'm open to any stylistic suggestions that might help. Or I suppose, this might just require us to over time become more familiar with flow syntax to make this more second nature to avoid confusion between string[], [string], and {[string]: ...}.

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Nov 24, 2017

Member

One thing we might consider doing is to switch from string[] to Array<string> for arrays. I've opened #987 to discuss it.

style: {[string]: string};

constructor(
value?: string,

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Nov 11, 2017

Member

This doesn't need to be optional, all call sites provide this information.

This comment has been minimized.

Copy link
@marcianx

marcianx Nov 13, 2017

Author Collaborator

Done.

constructor(children, attributes) {
class svgNode implements VirtualDomNode {
children: SvgChildNode[];
attributes: Attribute[];

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Nov 11, 2017

Member

In other places we provide attributes as attributes: {[string]: string}; we should do the same here. Not sure how much work is involved with that. If it looks like a lot feel free to add a TODO and tackle it later.

This comment has been minimized.

Copy link
@marcianx

marcianx Nov 13, 2017

Author Collaborator

Done.

@marcianx marcianx force-pushed the marcianx:flow-domTree branch from 6cbd82b to d4229bf Nov 13, 2017

@marcianx

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 13, 2017

Since a few other PRs made related files type-safe, there needed to be more edits to fix these files and others to address flow type errors.

}

} else {
// \cancel, \bcancel, or \xcancel
// Since \cancel's SVG is inline and it omits the viewBox attribute,
// its stroke-width will not vary with span area.

let attributes = [["x1", "0"]];
let attributes: {[string]: string} = {"x1": "0"};
const lines = [];

if (label !== "cancel") {

This comment has been minimized.

Copy link
@marcianx

marcianx Nov 13, 2017

Author Collaborator

Honestly, I don't understand the structure of the original logic. However, I've replicated it herein the conversion to object.

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Nov 24, 2017

Member

I think this is the "strikethrough" case. It's the reverse diagonal.

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Nov 24, 2017

Member

This probably could be simplified by moving not reusing the same attributes object for both diagonals. Maybe

if (/^x?cancel$/.test(label)) {
    lines.push(new domTree.lineNode({
        x1: 0,
        y1: 0,
        x2: "100%",
        y2: "100%",
    });
}

if (/^[bx]cancel$/.test(label)) {
    lines.push(new domTree.lineNode({
        x1: 0,
        y1: "100%",
        x2: "100%",
        y2: 0,
    });
}

This comment has been minimized.

Copy link
@marcianx

marcianx Nov 24, 2017

Author Collaborator

Ok, I'll simplify it as a tiny PR separately after doing more paranoid checks about what pathways can actually end up in the else block that contains this code.

span.style.height = height + "em";
spans.push(span);
if (numSvgChildren === 1) {
return {span, minWidth, height};

This comment has been minimized.

Copy link
@marcianx

marcianx Nov 13, 2017

Author Collaborator

You suggested this in the stretchy PR. I did this anyway to resolve some of the type errors. (It was otherwise interpreting the returned span to have type span | svgNode.

let paths;
let pathName;
let svgNode;
let span;

This comment has been minimized.

Copy link
@marcianx

marcianx Nov 13, 2017

Author Collaborator

I made all these variables as local as possible since it was getting difficult for me to otherwise reason about the area of influence of any assignment to any of these variables.

// This case probably shouldn't occur (this would mean the
// supsub was sending us a group with no superscript or
// subscript) but be safe.
return base;
} else {
bottom = options.fontMetrics().bigOpSpacing5 +

This comment has been minimized.

Copy link
@marcianx

marcianx Nov 13, 2017

Author Collaborator

I moved this to the top to make all the if statements positive so that type refinement could guarantee what was non-nullable in each if-then statement body.

supKern = Math.max(
options.fontMetrics().bigOpSpacing1,
options.fontMetrics().bigOpSpacing3 - supm.depth);
sup = {

This comment has been minimized.

Copy link
@marcianx

marcianx Nov 13, 2017

Author Collaborator

I coupled the elem (originally subm, supm) variable with the corresponding kern (originally subKern, supKern) so that type refinement (if checks) below could guarantee when both these values were set and usable.

@marcianx marcianx force-pushed the marcianx:flow-domTree branch 2 times, most recently from bd854eb to c8ab72a Nov 14, 2017

@marcianx marcianx force-pushed the marcianx:flow-domTree branch 3 times, most recently from ec6d7d6 to bba06fb Nov 23, 2017

"height": height + "em",
"viewBox": `0 0 ${viewBoxWidth} ${viewBoxHeight}`,
"preserveAspectRatio": aligns[i] + " slice",
});

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Nov 24, 2017

Member

This feels more idiomatic.

@kevinbarabash
Copy link
Member

left a comment

LGTM

@marcianx marcianx force-pushed the marcianx:flow-domTree branch from 371caab to 72ac651 Nov 24, 2017

@marcianx marcianx force-pushed the marcianx:flow-domTree branch from 72ac651 to 89eb72d Nov 24, 2017

@marcianx marcianx merged commit c8249c3 into KaTeX:master Nov 24, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@marcianx marcianx deleted the marcianx:flow-domTree branch Nov 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.