Katex: Migrate the codebase to ES2015 (and up) completely

Created on 5 Feb 2017  路  14Comments  路  Source: KaTeX/KaTeX

Do you have any plans to migrate the codebase to newer versions of javascript and use babel stage-4 or something similar?
It looks like that half of some files are written in es6 and half has the same old es5 syntax look at domTree.js for example. We definitely can migrate to es6 classes.
I also think using webpack 2 as a bundler is a good idea after addition of import and export support.
Generally I think having es6 codebase encourages more developers to contribute.
Finally I'm ready to finish the migration as a pull request if it is OK with you.

Most helpful comment

We could include the transform-class-properties babel plugin to enable static properties in classes. It's a stage 2 proposal, but it pretty stable.

All 14 comments

@HosseinAgha we definitely want to migrate to ES6. There are certain things we'd like to limit the use of because they use large polyfills or compile to code that's a lot bigger that the original. Basically everything that's covered by https://github.com/kevinbarabash/eslint-plugin-luddite.

A pull request would be greatly appreciated, but please leave any changes to the build system out. They shouldn't be necessary as I believe that we can continue to use babeblify and still use import/export. If there are other reasons to move to webpack then I'd like to discuss those separately.

For small classes like those in domTree, using ES6 class syntax might be a good thing. But for other use cases I'm less certain. When I started converting the codebase to ES6, I gave class notatioin a try for Parser, and noticed that it doesn't neccessarily make things better. We have several top level definitions intermixed with the method definitions in which they are used [1, 2, 3]. Afaik static members are not part of ES6, so there is no simple way to include these elements within the body of a class definition. Moving them away from the methods that use them feels like a bad idea. Since Parser is not the only class to contain intermixed code of this kind, I felt that we shouldn't abandon prototype assignments altogether. And if we keep using them, then I'm not really sure than mixing them with ES6 style class notation for simple classes is a good idea. Now people only have to know one style of writing a class, which I see as a benefit.

We could include the transform-class-properties babel plugin to enable static properties in classes. It's a stage 2 proposal, but it pretty stable.

@kevinbarabash I started to migrate the remaining files to use ES2015 classes -
I had to make some minor changes to .eslintrc and add transform-class-properties plugin.
Here is the first commit. all the tests are passing. I will continue if this style okay?

@HosseinAgha looks good so far. I assume the .eslintrc and babel plugin changes are in a different commit. Can you update Parser.js next and post those changes to see how it works out with transform-class-properties?

git's diffing algorithm leaves much to be desired.

Yes .eslintrc changes are here. I think we can squash some of the commits at the end.
I've already updated the Parser.js and all the tests are passing.
You can find the updated Parser.js here. Also the changes can be found at the end of this commit (commit is folded because of the long size)

I had to move all the constants before the prototype functions to a single block at the top the class.

Can you change those constants to static variables on the class so that they can be positioned closer to their use site? e.g.

class Parser {
    ...

    static sizeFuncs = [
        "\\tiny", "\\scriptsize", "\\footnotesize", "\\small", "\\normalsize",
        "\\large", "\\Large", "\\LARGE", "\\huge", "\\Huge",
    ];

    // A list of the style-changing functions, for use in parseImplicitGroup
    static styleFuncs = [
        "\\displaystyle", "\\textstyle", "\\scriptstyle", "\\scriptscriptstyle",
    ];

    parseImplicitGroup() {
        ...
    }

    ...
}

Sure.
I didn't do this because statics were not constants of course.

@kevinbarabash Katex uses pure node for jasmine tests and rely on node.js support of ES6 features so I couldn't make the static work with existing configuration.
so I can think of 3 solutions:

  • Update tests to compile modules to es5 before doing anything
  • Use static-accessor-properties whenever we wanted to use a static property (It works everywhere but using it for our constants looks a bit awkward)
  • Abandon migrating static functions altogether and use same old ES5 syntax for that

@HosseinAgha have a look at the "Jasmine" section on http://babeljs.io/docs/setup/#installation.

Fantastic! Thanks.

@kevinbarabash I recently found out that for the development server (server.js) you serve ES6 untranspiled code to the browser and a transpiled version of that on a /babel route.
I think we should transpile the code by default and forget about the /babel route because import and export support is still experimental in all browsers.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

trollanfer picture trollanfer  路  5Comments

jason-s picture jason-s  路  3Comments

HughGrovesArup picture HughGrovesArup  路  4Comments

shaunc picture shaunc  路  4Comments

sophiebits picture sophiebits  路  3Comments