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 buildHTML to @flow. #1408

Merged
merged 3 commits into from Jun 10, 2018

Conversation

@marcianx
Copy link
Collaborator

commented Jun 5, 2018

Looking through this PR, some of these changes to buildHTML...seem ancient, and I was surprised they were not in yet. Looking back, I just realized that the last 21 PRs I sent were just so that this PR could finally finish. O_o

buildMathML should come in soon as well.

@marcianx marcianx requested a review from kevinbarabash Jun 5, 2018

@codecov

This comment has been minimized.

Copy link

commented Jun 5, 2018

Codecov Report

Merging #1408 into master will decrease coverage by 0.07%.
The diff coverage is 81.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1408      +/-   ##
==========================================
- Coverage   82.37%   82.29%   -0.08%     
==========================================
  Files          77       77              
  Lines        4362     4382      +20     
  Branches      760      763       +3     
==========================================
+ Hits         3593     3606      +13     
- Misses        666      673       +7     
  Partials      103      103
Impacted Files Coverage Δ
src/stretchy.js 79.54% <ø> (ø) ⬆️
src/functions/delimsizing.js 65.16% <0%> (-1.5%) ⬇️
src/functions/accent.js 90.47% <100%> (ø) ⬆️
src/functions/supsub.js 97.89% <100%> (ø) ⬆️
src/domTree.js 63.82% <50%> (-0.62%) ⬇️
src/utils.js 68.75% <75%> (+0.56%) ⬆️
src/buildHTML.js 97.05% <97.05%> (+0.13%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1dc2612...cf5ef49. Read the comment docs.

@marcianx marcianx force-pushed the marcianx:flow-html branch from f0a5d6f to fece498 Jun 5, 2018

expression: *[],
options: Options,
isRealGroup: boolean,
surrounding: [?string, ?string] = [null, null],

This comment has been minimized.

Copy link
@ylemkimon

ylemkimon Jun 5, 2018

Member

surroundings are one of DomTypes.

This comment has been minimized.

Copy link
@marcianx

marcianx Jun 5, 2018

Author Collaborator

Ah, thanks! Fixed.

export const buildExpression = function(expression, options, isRealGroup,
surrounding = [null, null]) {
export const buildExpression = function(
expression: *[],

This comment has been minimized.

Copy link
@ylemkimon

ylemkimon Jun 7, 2018

Member

Is it currently impossible to type expression and

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Jun 8, 2018

Member

expression: AnyParseNode[] should work.

This comment has been minimized.

Copy link
@marcianx

marcianx Jun 8, 2018

Author Collaborator

Sorry, sloppy me. I didn't go back through the stuff I'd already added types for way back when. Fixed.

@@ -215,14 +258,19 @@ export const makeNullDelimiter = function(options, classes) {
* function for it. It also handles the interaction of size and style changes
* between parents and children.
*/
export const buildGroup = function(group, options, baseOptions) {
export const buildGroup = function(
group: *,

This comment has been minimized.

Copy link
@ylemkimon

ylemkimon Jun 7, 2018

Member

group and

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Jun 8, 2018

Member

This can be group: ?AnyParseNode

This comment has been minimized.

Copy link
@marcianx

marcianx Jun 8, 2018

Author Collaborator

Done

@@ -273,7 +321,7 @@ function buildHTMLUnbreakable(children, options) {
* Take an entire parse tree, and build it into an appropriate set of HTML
* nodes.
*/
export default function buildHTML(tree, options) {
export default function buildHTML(tree: *, options: Options): DomSpan {

This comment has been minimized.

Copy link
@ylemkimon

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Jun 8, 2018

Member

This can be tree: AnyParseNode[].

This comment has been minimized.

Copy link
@marcianx

marcianx Jun 8, 2018

Author Collaborator

Done

@kevinbarabash
Copy link
Member

left a comment

I love how static typing really shines a light on all the awkward things we're doing.

@@ -40,13 +51,28 @@ const isBinRightCanceller = function(node, isRealGroup) {
}
};

const styleMap = {
type StyleString = "display" | "text" | "script" | "scriptscript";
const styleMap: {[StyleString]: StyleInterface} = {

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Jun 8, 2018

Member

This could be simplified by defining StyleString after styleMap similar to how you defined DomType from DomEnum.

This comment has been minimized.

Copy link
@marcianx

marcianx Jun 8, 2018

Author Collaborator

Done.

export const buildExpression = function(expression, options, isRealGroup,
surrounding = [null, null]) {
export const buildExpression = function(
expression: *[],

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Jun 8, 2018

Member

expression: AnyParseNode[] should work.

for (let i = 0; i < expression.length; i++) {
const output = buildGroup(expression[i], options);
if (output instanceof domTree.documentFragment) {
rawGroups.push(...output.children);
// $FlowFixMe: should consist entirely of `symbolNode`s and `span`s.

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Jun 8, 2018

Member

This $FlowFixMe isn't necessary. If it's more precise to type children as (SymbolNode | span)[] then leave a TODO instead.

This comment has been minimized.

Copy link
@marcianx

marcianx Jun 8, 2018

Author Collaborator

Done

...rawGroups.filter(group => group && group.classes[0] !== "mspace"),
surrounding[1] && makeSpan([surrounding[1]], [], options),
surrounding[1] ? makeSpan([surrounding[1]], [], options) : null,

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Jun 8, 2018

Member

I wish we knew the atom type for a group before we call its HTML builder. Maybe that's something that will fall out of moving to an intermediate representation of boxes.

This comment has been minimized.

Copy link
@marcianx

marcianx Jun 8, 2018

Author Collaborator

Sorry, I'm not quite sure I understand your statement and what consequent simplification of code you had in mind.

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Jun 10, 2018

Member

No action necessary here. Currently we only know the atom type for a group until after we build the HTML for it. Eventually, it would be nice if we could do:

ParseNode tree => LayoutNode tree => HtmlNode tree

where

LayoutNode =
   | Box
   | Rule
   | Kern
   | Glue
   | Glyph

The benefit of this would be being able to have different rendering backends so that we could do any of the following:

LayoutNode tree => HtmlNode tree
LayoutNode tree => SvgNode tree
LayoutNode tree => canvas render

Having a tree of LayoutNodes would allow us to do any post-processing such as bin cancellation on the LayoutNode tree once instead of having to have separate copies of the code for each of the final trees.

];

// Before determining what spaces to insert, perform bin cancellation.
// Binary operators change to ordinary symbols in some contexts.
for (let i = 1; i < nonSpaces.length - 1; i++) {
const left = getOutermostNode(nonSpaces[i], "left");
// $FlowFixMe: Only the first and last can be nullable.
const nonSpacesI: HtmlDomNode = nonSpaces[i];

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Jun 8, 2018

Member

I think removing the $FlowFixMe and doing const nonSpacesI: HtmlDomNode = (nonSpaces[i]: any); would be better b/c at least nonSpacesI would be typed as a HtmlDomNode instead of having its type erased b/c of the $FlowFixMe.

This comment has been minimized.

Copy link
@marcianx

marcianx Jun 8, 2018

Author Collaborator

$FlowFixMe just allows the assignment; flow would still type nonSpaces as HtmlDomNode and assume its type as that in the subsequent lines. In any case, I added a general-purpose assert() method in utils since I've wanted it more than once before and removed the $FlowFixMe.

if (!group) {
return makeSpan();
}

if (groupBuilders[group.type]) {
// Call the groupBuilders function
let groupNode = groupBuilders[group.type](group, options);
// $FlowFixMe
let groupNode: HtmlDomNode = groupBuilders[group.type](group, options);

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Jun 8, 2018

Member

I tried export const _htmlGroupBuilders: {[NodeType]: HtmlBuilder<NodeType>} = {}; but that didn't fix the $FlowFixMe. :(

This comment has been minimized.

Copy link
@marcianx

marcianx Jun 8, 2018

Author Collaborator

HtmlBuilder<NodeType> isn't the type union of all possibilities since it's not variant w.r.t. NodeType (by virtue of us computing NodeValue from it). Really, it should be {[NodeType]: AnyHtmlBuilder}. But it's probably not worth the effort. Almost all the typesafety we get is from the definitions defineFunction and definedEnvironment. Almost nothing else touches _htmlGroupBuilders; the couple methods that do expose a very strongly-typed signature.

@@ -273,7 +321,7 @@ function buildHTMLUnbreakable(children, options) {
* Take an entire parse tree, and build it into an appropriate set of HTML
* nodes.
*/
export default function buildHTML(tree, options) {
export default function buildHTML(tree: *, options: Options): DomSpan {

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Jun 8, 2018

Member

This can be tree: AnyParseNode[].

@@ -19,7 +20,8 @@ export const htmlBuilder: HtmlBuilderSupSub<"accent"> = (grp, options) => {
let base: AnyParseNode;
let group: ParseNode<"accent">;

const supSub = checkNodeType(grp, "supsub");
// $FlowFixMe: Maybe due to https://github.com/facebook/flow/issues/6379
const supSub: ?ParseNode<"supsub"> = checkNodeType(grp, "supsub");

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Jun 8, 2018

Member

As we able to remove this $FlowFixMe after making the other changes I suggested above.

This comment has been minimized.

Copy link
@marcianx

marcianx Jun 8, 2018

Author Collaborator

Done

// Property `isMiddle` not defined on `span`. See comment in
// "middle"'s htmlBuilder.
// $FlowFixMe
const isMiddle: IsMiddle = middleDelim.isMiddle;

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Jun 8, 2018

Member

I find it really weird that isMiddle is not a boolean... we should probably change this property to be called simply middle. Maybe add a TODO to make that change separately.

This comment has been minimized.

Copy link
@marcianx

marcianx Jun 8, 2018

Author Collaborator

I added a TODO comment in htmlBuilder that the comment above references.

@@ -175,7 +175,7 @@ defineFunctionBuilders({
}

// Wrap the supsub vlist in a span.msupsub to reset text-align.
const mclass = html.getTypeOfDomTree(base) || "mord";
const mclass = html.getTypeOfDomTree(base, "right") || "mord";

This comment has been minimized.

Copy link
@kevinbarabash

kevinbarabash Jun 8, 2018

Member

This change isn't necessary after re-enabling the param default.

This comment has been minimized.

Copy link
@marcianx

marcianx Jun 8, 2018

Author Collaborator

I added it because it wasn't obvious to me when it's omitted that the default behavior is "right". Is there any obvious reason to make "right" the default over, say, "left"? If not, I think having it explicitly would aid readability.

@marcianx marcianx force-pushed the marcianx:flow-html branch from ebdcaf5 to cf5ef49 Jun 9, 2018

@kevinbarabash
Copy link
Member

left a comment

LGTM

@kevinbarabash kevinbarabash merged commit 251283f into KaTeX:master Jun 10, 2018

3 of 4 checks passed

codecov/project 82.29% (-0.08%) compared to 1dc2612
Details
ci/circleci: chrome Your tests passed on CircleCI!
Details
ci/circleci: firefox Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kevinbarabash

This comment has been minimized.

Copy link
Member

commented Jun 10, 2018

Looking through this PR, some of these changes to buildHTML...seem ancient, and I was surprised they were not in yet. Looking back, I just realized that the last 21 PRs I sent were just so that this PR could finally finish. O_o

I appreciate how deliberate you've been about these changes. I'm glad that we're getting a place where we're able to do larger refactorings safely.

buildMathML should come in soon as well.

Looking forward to it.

@marcianx marcianx deleted the marcianx:flow-html branch Jun 10, 2018

@ylemkimon ylemkimon referenced this pull request Aug 18, 2018
1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.