[WIP] build: migrate to Typescript #2326

ylemkimon · 2020-07-20T13:22:49Z

Closes #2110.

Fix ~300 errors
Enable typescript-eslint

References:

Any comments and helps are appreciated! Please feel free to open a PR or commit directly against this branch.

ronkok · 2020-07-21T16:23:17Z

Wow.

kevinbarabash · 2020-08-01T18:49:42Z

This is exciting!

kevinbarabash

Thanks for doing this! It so nice to see some those comments reference flow bugs go away. It's too bad about all of the test failures. I'll check out your branch sometime this week and try to make some progress with these failing tests.

kevinbarabash · 2020-08-01T18:50:16Z

katex.ts


 import {defineSymbol} from './src/symbols';
 import {defineMacro} from './src/macros';
 import {setFontMetrics} from './src/fontMetrics';

-declare var __VERSION__: string;
+declare const __VERSION__: string;


I didn't realize the TS added support for this. Cool!

kevinbarabash · 2020-08-01T18:51:24Z

katex.ts

@@ -88,7 +81,7 @@ const generateParseTree = function(
 * error message.  Otherwise, simply throws the error.
 */
 const renderError = function(
-    error,
+    error: any,


Good call making types for function params required. Even if we need to add a bunch of anys it makes it easier to see places where the typing could be better.

kevinbarabash · 2020-08-01T18:52:07Z

src/MacroExpander.ts

@@ -30,7 +28,7 @@ export const implicitCommands = {
 export default class MacroExpander implements MacroContextInterface {
    settings: Settings;
    expansionCount: number;
-    lexer: Lexer;
+    lexer!: Lexer;


What does the ! modifier do?

@kevinbarabash ! is a non-null assertion operator, since feed, which sets the lexer, is always called.

kevinbarabash · 2020-08-01T18:54:10Z

src/MacroExpander.ts

@@ -317,7 +311,7 @@ export default class MacroExpander implements MacroContextInterface {
     * Returns the expanded macro as a reversed array of tokens and a macro
     * argument count.  Or returns `null` if no such macro.
     */
-    _getExpansion(name: string): ?MacroExpansion {
+    _getExpansion(name: string): MacroExpansion | null {


If we want to match flow semantics ?T should be to T | null | undefined. I'm not sure if this is what's causing the 300 errors. If it isn't though T | null is probably better since it's more restrictive. FTR, I don't think that we should be trying to maintain flow semantics once we switch to TypeScript.

kevinbarabash · 2020-08-01T18:54:46Z

src/Options.ts

+    style: StyleInterface,
+    color?: string | void,
+    size?: number,
+    textSize?: number,
+    phantom?: boolean,
+    font?: string,
+    fontFamily?: string,
+    fontWeight?: FontWeight,
+    fontShape?: FontShape,
+    sizeMultiplier?: number,
+    maxSize: number,
+    minRuleThickness: number


I thought that ; was supposed to be used in types to separate properties in TypeScript.

It´s an Object. The values in an Object are seperated with a ,.

kevinbarabash · 2020-08-01T19:13:30Z

src/mathMLTree.ts


-    constructor(type: MathNodeType, children?: $ReadOnlyArray<MathDomNode>) {
+    constructor(type: MathNodeType, children?: Array<MathDomNode>) {


Can we not use ReadonlyArray here?

kevinbarabash · 2020-08-01T19:14:04Z

src/spacingData.ts

-// Making the type below exact with all optional fields doesn't work due to
-// - https://github.com/facebook/flow/issues/4582
-// - https://github.com/facebook/flow/issues/5688
-// However, since *all* fields are optional, $Shape<> works as suggested in 5688
-// above.


Not sad to see this comment go bye-bye. 😄

kevinbarabash · 2020-08-01T19:15:11Z

src/stretchy.ts

@@ -245,7 +245,7 @@ const svgSpan = function(
            let widthClasses;
            let aligns;
            if (numSvgChildren === 1) {
-                // $FlowFixMe: All these cases must be of the 4-tuple type.


It would be good to maintain this comment if it's still valid under TypeScript.

kevinbarabash · 2020-08-01T19:16:03Z

src/utils.ts

-/**
- * Provide a default value if a setting is undefined
- * NOTE: Couldn't use `T` as the output type due to facebook/flow#5022.
- */


kevinbarabash · 2020-08-01T19:17:30Z

tsconfig.json

+    // "emitDecoratorMetadata": true,         /* Enables experimental support for emitting type metadata for decorators. */
+
+    /* Advanced Options */
+    "skipLibCheck": true,                     /* Skip type checking of declaration files. */


I don't know why this isn't the default. I have a personal project I'm using TS for and it took a lot of search to find out this not using this setting what was making build slow.

@kevinbarabash This is default from tsc --init

ylemkimon · 2020-08-02T18:22:32Z

@kevinbarabash Thank you for comments and the great tool! Most of changes are done by flow-to-ts 😄 I didn't have to do a thing. ~~I'll file issues I've encountered there.~~

ylemkimon · 2020-08-02T18:30:15Z

It seems somehow I ran an old version of flow-to-ts. I'll update the PR, once we have stable codebase with recent PRs merged.

kevinbarabash · 2020-08-02T18:43:35Z

I'll update the PR, once we have stable codebase with recent PRs merged.

Sounds good. I checked out the branch and started trying to fix things. I got the error count down to 253 but it's kind of slow going. Maybe we should add @ts-ignores with a TODO to fix it later for any TypeScript error that isn't super easy to fix. That way we don't have to worry about the PR getting stale and we can fix the errors in smaller chunks after it gets merged.

mhchem · 2020-09-02T15:11:24Z

There is a TypeScript version of the mhchem parser. Using it for this pull request might save you some time.

ylemkimon added 8 commits July 20, 2020 21:30

Migrate to Typescript

53ea0b1

Convert Flowtype to Typescript using flow-to-ts

4a608cb

Convert contrib to Typescript

08f060e

Fix ts errors

c005081

Run ts in test

0334a9e

Fix code style

45b7957

Run ts in CircleCI

262fec1

Update ts

755a791

ylemkimon added the help wanted label Jul 20, 2020

ylemkimon added 2 commits July 24, 2020 04:52

Enable Rollup bundling ts

870af0a

Remove flow

91a99db

ylemkimon mentioned this pull request Jul 25, 2020

chore: migrate to Yarn 2 #2316

Merged

Merge branch 'master' into typescript

a8f38e4

kevinbarabash reviewed Aug 1, 2020

View reviewed changes

ronkok mentioned this pull request Aug 9, 2020

feat: Support {CD} #2396

Merged

ylemkimon changed the title ~~[WIP] chore: migrate to Typescript~~ [WIP] build: migrate to Typescript Aug 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] build: migrate to Typescript #2326

[WIP] build: migrate to Typescript #2326

ylemkimon commented Jul 20, 2020 •

edited

ronkok commented Jul 21, 2020

kevinbarabash commented Aug 1, 2020

kevinbarabash left a comment

kevinbarabash Aug 1, 2020

kevinbarabash Aug 1, 2020

kevinbarabash Aug 1, 2020

ylemkimon Aug 2, 2020

kevinbarabash Aug 1, 2020

kevinbarabash Aug 1, 2020

korki43 Sep 14, 2020

kevinbarabash Aug 1, 2020

kevinbarabash Aug 1, 2020

kevinbarabash Aug 1, 2020

kevinbarabash Aug 1, 2020

kevinbarabash Aug 1, 2020

ylemkimon Aug 2, 2020

ylemkimon commented Aug 2, 2020 •

edited

ylemkimon commented Aug 2, 2020

kevinbarabash commented Aug 2, 2020

mhchem commented Sep 2, 2020


		constructor(type: MathNodeType, children?: $ReadOnlyArray<MathDomNode>) {
		constructor(type: MathNodeType, children?: Array<MathDomNode>) {

[WIP] build: migrate to Typescript #2326

Are you sure you want to change the base?

[WIP] build: migrate to Typescript #2326

Conversation

ylemkimon commented Jul 20, 2020 • edited

ronkok commented Jul 21, 2020

kevinbarabash commented Aug 1, 2020

kevinbarabash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ylemkimon commented Aug 2, 2020 • edited

ylemkimon commented Aug 2, 2020

kevinbarabash commented Aug 2, 2020

mhchem commented Sep 2, 2020

ylemkimon commented Jul 20, 2020 •

edited

ylemkimon commented Aug 2, 2020 •

edited