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

Research Closure Compiler + Typescript Support #134

Closed
traviskaufman opened this issue Dec 19, 2016 · 11 comments
Closed

Research Closure Compiler + Typescript Support #134

traviskaufman opened this issue Dec 19, 2016 · 11 comments

Comments

@traviskaufman
Copy link
Contributor

traviskaufman commented Dec 19, 2016

By the time MDC-Web reaches beta, we need to provide support for typed ECMA variants. Specifically, we need support for Closure Compiler (Closure) and Typescript (TS). This is the issue where all research around our strategy for supporting Closure + TS should be done.

Closure Compiler Support

Google Closure Compiler support is required in order to support the Google projects and properties which are built around this toolchain. Concretely, MDC-Web must be able to compile with ADVANCED_OPTIMIZATIONS enabled, and produce no errors or warnings. There are implications for internal support as well, but that is outside the scope of this issue.

Verification Plan

  • Introduce a "build test", e.g. npm run test:closure which runs closure on all packages, but does not output anything. This can be done using closure's --checks_only flag. The advantage here is that nothing about our build system changes.
  • OPTIONAL: Switch out uglifyJS for the closure compiler webpack plugin, and use that to minify our code. The advantage here is we not only get type-checking but we also take advantage of closure's extremely sophisticated optimization techniques. The disadvantage is that it's way slower to optimize, and we may have to restructure some of our code for it to work correctly.

Typescript support

Typescript is angular's de facto language choice. It also seems to be the most popular typed ECMAScript variant. For these reason, I believe it's important to provide first-class support for Typescript users wanting to use MDC-Web. Concretely, MDC-Web should provide type declaration files for all components, foundations, and adapters.

The type declaration files can live in the component package directory, and should be called index.d.ts This will allow typescript's module resolution algorithm to automatically discover these typings. Furthermore, having type declarations should become a requirement for all components moving forward. The TS handbook as a module declaration file template that we could use as a starting point for our TS modules.

Verification Plan

I'm not sure how to validate type declaration files alone, so more research has to be done on this.

Implementation Options

There are a few implementation options which I will discuss below. Any additional implementation options surfaced by the community should be added to this section. I personally think all of these options should be experimented with, but we could rule some out via discussion on this issue.

Closure-annotated source files + manual typings

All source files are annotated using JSDoc for the closure compiler.

Type declaration files are manually authored in addition to adding closure compiler annotations.

A page within docs/ could be created outlining any non-standard practices within our source code that are closure-specific (e.g. using expressions for @typedefs, using dummy classes for @record types, etc.)

Pros

  • Requires the least amount of changes to tooling and infrastructure
  • Source code will be idiomatic for closure compiler users, and will yield optimal closure compiler optimizations.
  • Type declarations will be idiomatic for TS users.

Cons

  • More work for developers to implement components.
  • Duplication of effort specifying same types for two different type systems.
  • Type declarations and closure annotations prone to go out-of-sync. Tooling may be needed to verify that changes to JSDocs are reflected by changes to type declarations, and vice-versa.

Closure-annotated source files + Clutz

All source files are annotated using JSDoc for the closure compiler.

Angular's Clutz tool is used to emit type declaration files from closure-annotated source files. This can be added as part of a pre-commit hook.

Pros

  • Source files remain in ES2015, and ensure highest level of optimization from Closure
  • No duplication of effort maintaining both Closure annotations and type decls
  • Source files are idiomatic for closure compiler users
  • Speculatively, it seems that TS can support Closure's type system much better than Closure can support TS's type system. Thus, transpilation from Closure to TS may be more high-quality than the other way around (see below).

Cons

  • Clutz requires Java, as well as some extra set up on our end
  • Unsure of the quality of type declarations emitted by clutz. Research has to be done on this.

Typescript Source files + Tsickle

Instead of writing source files in ES2015, source files are written using Typescript.

Before publishing to npm, and extra build step will be added to transpile them to their ES2015 sources using CommonJS as its module system. This would ensure the built source files would work with module loading systems such as webpack. Alternatively, we could preserve the ES2015 imports.

Angular's Tsickle could be used to annotate the source files for closure.

The TS compiler could output proper declaration files for our project, which could be added to version control since the declarations files would reflect the public API.

Pros

  • We get all of the nice language tooling around working on large-scale TS codebases
  • TS becomes our single source of truth. Declarations and closure-compatible JS could all be emitted from our source files, reducing duplication of effort and number of manual tasks needed to implement a component.
  • Source code feels idiomatic to the closure community

Cons

  • Doing this would require a rewrite of all of current components to TS, as well as all agreeing on a new source language that we're using.
  • Doing this would require an overhaul to our build system
  • The use of TS - while popular - still means using a base language different from plain ECMAScript, which is what we strive for in order to facilitate the maximum amount of community contribution.
  • Tsickle has many open issues denoting that the quality of the transpilation of TS sources might not be up-to-par with the quality of hand-written JSDoc. This may preclude its use within internal apps since it would lead to poorer optimization. In my limited experience with Tsickle, it's been okay but we'd have to test this out on a non-trivial component (such as @material/ripple before deeming it viable).
@WillsB3
Copy link
Contributor

WillsB3 commented Jan 24, 2017

Yay for Closure Compiler 👍

How do you see developers using MDC-Web with their applications build steps? Are they to just include a pre-compiled distributable version which they should not run through Closure Compiler as part of their application build step, in which case won't MDC-Web need to provide an externs file? or will users be able to included the MDC-Web source and have it be compiled (using ADVANCED_OPTIMIZATIONS as mentioned) as part of their overall application build process?

@traviskaufman
Copy link
Contributor Author

How do you see developers using MDC-Web with their applications build steps?

The most probable way is through webpack/gulp/grunt via google-closure-compiler-js.

The trickiest part is module resolution. While all of our code will be compilable via closure, there's still the matter of resolving ES2015 import statements. Because closure has no notion of node's module resolution mechanics, the code will have to be pre-processed in such a way that its dependencies are resolved. I'm currently looking into how we're going to do this in order to test that our code is compilable via closure, as well as to produce the correct Typescript typings via clutz if that's possible.

will users be able to included the MDC-Web source and have it be compiled (using ADVANCED_OPTIMIZATIONS as mentioned) as part of their overall application build process?

As alluded to above, yes, as long as they make closure aware of how to resolve module dependencies within MDC-Web.

@WillsB3
Copy link
Contributor

WillsB3 commented Jan 24, 2017

Great - thanks for the info 👍

@traviskaufman
Copy link
Contributor Author

Looks like Closure Compiler is working on adding node module resolution capability, which will make some of the work we have to do around getting modules to get recognized much easier. google/closure-compiler#2130

@traviskaufman
Copy link
Contributor Author

Filed google/closure-compiler#2257 regarding using identifiers from re-exported modules as types. This would create issues for us since we re-export all of our foundation classes through index.js.

@traviskaufman
Copy link
Contributor Author

traviskaufman commented Feb 3, 2017

Research Findings

TL;DR For simply being able to be compilable by closure, we'd have to smooth over some rough edges but it's definitely doable. Clutz, however, is inadequate for creating public type declarations, but there are feasible solutions for implementing these translations ourselves.

This comment documents my findings on researching how to properly provide closure + TS support. All of my work can be found on the experimental/closure-research branch.

Approach

So for this, I decided to go with the Closure-annotated source files + Clutz approach outlined in the original issue. There were a few reasons for this:

  • Tsickle, in our experience, has not produced typings adequate enough to take full advantage of closure's advanced optimizations mechanisms. This would mean flagship google products may consider our code to be a non-starter, especially if the only thing we provided would be externs.
  • TS's type system can handle pretty much anything within Closure's type system, but it's definitely not the same the other way around. Thus, going from Closure to TS rather than TS to closure seems like a safer bet.
  • The team was pretty against rewriting in Typescript, since it would be a pretty significant undertaking from an infrastructure perspective. It was more like a "nuclear option" they would consider.
  • The closure compiler team is willing to collaborate with us no addressing issues that arise as we begin annotating all of our files. They are invested in making closure as correct and efficient as possible, and we provide a great facility for finding and fixing errors that wouldn't commonly arise within other dependent projects.

To prove the viability of this option, I attempted to compile our icon-toggle package via closure, and then generate type definitions for it via clutz. Icon-toggle is a feature-rich component that depends on the mdc-base and mdc-ripple packages, and has a non-trivial adapter API. I felt it would be a solid litmus test for the viability of this approach.

Implementation Notes

Setting up closure - ✅

Closure provides two packages via npm - google-closure-compiler, which is a standalone jar of the Java compiler, as well as google-closure-compiler-js, which is a GWT-transpiled version of closure compiler.

For this experiment, I chose to go with google-closure-compiler. On the google-closure-compiler-js README, it states:

This is an experimental release- some features are not available and performance may not be on-par with the Java implementation.

Which deterred me from considering it as highly. But the real reason is that internally, teams will be building our code using the Java version of the closure compiler, and I wanted this to be as close to the real-world scenario for using closure as possible. The drawback of this approach is that it requires Java. However, most modern computers ship with Java, and installation on TravisCI seems easy enough.

Furthermore, I chose not to use closure with webpack. Again, this mostly has to do with how closure is used within Google. Unlike with Webpack, where closure seems to be used solely to minify and optimize scripts, in Google the closure compiler both transpiles ES2015 sources as well as optimizes it. Again, in an effort to be as aligned with the predominant user of closure as possible, I wanted to simulate what it would be like if it was being built by bazel. This means that our CI system could eventually incorporate these closure build tests for all of our packages, giving us a type checking system and ensuring that PRs never cause our closure builds to fail.

So to install it I simply ran:

npm install -D google-closure-compiler

Which means closure is called by executing java -jar node_modules/google-closure-compiler/compiler.jar from the MDC-Web root dir.

Potential Problems

None!

Setting up Clutz - ❌

Setting up Clutz was not easy, and required manual effort. First, since there is no binary distribution, it has to be built from source. This requires gradle to be installed on the host OS, which I did via homebrew.

brew install gradle

Then I had to build the library and ensure that the built binary is on your path. Building clutz also requires the user to install clang-format in the same directory as clutz via npm, or else it won't build.

cd /path/to/clutz
npm i clang-format
gradle build installDist

export PATH=$PATH:$PWD/build/install/clutz/bin

Potential Problems

Setting up clutz on a machine is a very manual process at this point. This means we would either have to write scripts to bootstrap clutz for contributors, or thoroughly document how to set up clutz. Both have major drawbacks with regards to maintaining the procedure for setting up Clutz in case the way it's built or distributed changes. Another solution could be to work with the Clutz team to begin providing binary distros.

Compilation via Closure - 🔜 ✅

Since the goal of this experiment was to test the viability of compiling mdc-icon-toggle via closure, I hacked together a script to do so.

The script essentially creates a .closure-tmp dir, moves icon-toggle and all of its JS dependencies into a clean directory structure that closure can understand, rewrites import statements so that closure can understand them. and then runs the compiler in typechecks-only mode to verify that the compilation was successful. Here is the output from running the script:

traviskaufman in ~/dev/material-components-web on experimental/closure-research ● λ ./scripts/closure-test.sh
Prepping icon toggle for possible JS compilation
Rewriting import statements via sed. IRL this would be done using AST transformations
Compiling JS
java -jar node_modules/google-closure-compiler/compiler.jar --compilation_level ADVANCED --js .closure-tmp/packages/**/*.js --language_out ECMASCRIPT5_STRICT --dependency_mode STRICT --entry_point .closure-tmp/packages/mdc-icon-toggle/index --js_module_root .closure-tmp/packages --jscomp_off accessControls --checks_only

Compilation successful!

Making icon-toggle compilable

Icon-toggle was a great example component because of its reliance on both MDC-Base and MDC-Ripple. Because it relied on these components, it meant that they also had to be closure-compilable as well. Here's what I came up with:

  • You'll notice a new file - adapter.js - within the mdc-icon-toggle and mdc-ripple packages, which look similar to this generic example:
/** @record */
export default class MDCComponentAdapter {
  /**
   * @param {string} className
   */
  addClass(className) {}

  /** 
   * @return {string}
   */
  getAttr() {}

  // ...
}

These adapter.js files export one default class specifying a closure structural interface. This is a perfect fit for our adapters since that is essentially what they are. Moving forward, all JS components would have an adapter.js file where the component's adapter definitions.

  • In the mdc-base package, MDCFoundation has changed such that it is now parameterized via /** @template T */, and same for MDCComponent. Thus, our two based types are MDCFoundation<T>, and MDCComponent<T>. The generic type T in MDCFoundation refers to the adapter that the foundation is given, and the T in MDCComponent refers to the foundation class that the component is either given or instantiates.

Thus, the MDCFoundation now looks like:

/** @template T */
export default class MDCFoundation {
  // ...

  /**
   * @param {T=} adapter
   */
  constructor(adapter = {}) {
    /** @protected {!T} */
    this.adapter_ = /** @type {!T} */ (adapter);
  }

  // ...
}

And MDCComponent now looks like:

/**
 * @template T
 */
export default class MDCComponent {
  // ...

  /**
   * @param {!Element} root
   * @param {T=} foundation
   * @param {...?} args
   */
  constructor(root, foundation = this.getDefaultFoundation(), ...args) {
    /** @protected @const {!Element} */
    this.root_ = root;
    this.initialize(...args);
    /** @protected @const {T} */
    this.foundation_ = foundation;
    // ...
  }

  // ...
}

Initial problems with compilation

Export aliasing

See google/closure-compiler#2257. This is an easy enough problem to work around, considering that we can simply import files from exactly where they are exported, rather than through their aliases. Also note that we can still export aliases for our external users, we just can't use them internally. This limitation for ourselves must be documented for our contributors.

NodeJS import system (fixed)

See google/closure-compiler#2130. Before this PR, closure could not import any of our code without us re-writing the import statements, similar to how we are doing it in this prototype. With this PR merged, this will no longer be an issue since closure can now use Node's module resolution algorithm to resolve modules 🎉

Type-checking issues in getters/setters

See google/closure-compiler#2261. Because we make heavy use of getters and setters within our vanilla components, this means that we need to disable accessControls checks by the compiler in order for our code to compile correctly. After talking with the closure team, the fix seems relatively straightforward, and something we could implement ourselves if the core team does not have time to get around to it.

Strange-looking code for those who have never seen closure

If you've never annotated JS for closure before, things like this:

/**
 * @typedef {{
 *   isActivated: boolean,
 *   wasActivatedByPointer: boolean,
 *   wasElementMadeActive: boolean,
 *   activationStartTime: number,
 *   activationEvent: ?Event
 * }}
 */
let ActivationState; // eslint-disable-line no-unused-vars

Probably look extremely strange. We should definitely maintain some sort of documentation that helps onboard our contributors to the idiosynchrasies of closure, and perhaps provide a "crash-course" of sorts regarding what they need to know about the compiler in order to productively contribute to our code-base. Basically, we want to make it easy as possible for them to contribute without having closure knowledge gaps get in the way.

Potential Problems

Closure's New Type Inference (NTI) system

Closure is working on a "new type inference" system which introducer stricter checks and hardened static analysis from the compiler. While this will prove useful, the way in which we structure our code currently does not adhere to it. For example, attempting to de-reference a parameter of a template type {!T} will yield an error because the type-checker cannot guarantee that {!T} will be an object. Because closure does not support bounded generics (e.g. <? extends !Object>, there would have to be a runtime check to ensure that the type is an object, which is completely suboptimal.

From speaking with the team, it seems that NTI is still being actively developed, and although there are a few apps using it, not very many are. To that end, they thought it would be fine if for now our code was compilable using the old type system.

Dead code from @record classes and @typedef expressions

A relatively bigger issue that we face with closure is the exporting and importing of @record types and @typedef types. Since we import these in our foundation and adapter files, webpack includes them in our dist'd JS builds, even though they serve no functional purpose. This has the potential to increase our code size and add unnecessary weight to our distributions.

I believe that one way we could investigate solving this would be to write a babel plugin that knows how to strip these import statements out of our source files, similar to https://www.npmjs.com/package/babel-plugin-transform-flow-strip-types. As long as we adhere to conventions about where we put our adapters and what the code looks like, they can be easily identified within import statements and just as easily stripped out. This will allow our source code to be compilable by closure but not impose any code size overhead for our built sources via webpack.

Typedefs via Clutz - ❌

Once I was able to get closure compilation working successfully, my next goal was to emit a correct Typescript declaration file for mdc-icon-toggle via Clutz. However, this did not prove too fruitful.

The command I tried running was as follows:

clutz .closure-tmp/packages/**/*.js --closure_entry_points .closure-tmp/packages/mdc-icon-toggle/index.js

This generated a litany of errors, mostly related to the fact that I was missing closure externs, which clutz seems to require. So I manually downloaded the closure externs and placed them within .closure-tmp/externs. Then I re-ran the command:

clutz .closure-tmp/packages/**/*.js --closure_entry_points .closure-tmp/packages/mdc-icon-toggle/index.js --externs $(find .closure-tmp/externs -name "*.js")

I still got a bunch of errors, but this time they all circled around module names not being able to be resolved correctly. From what I could tell, it seems like clutz expects you to use goog.provide()/goog.module() in your code, and doesn't really take to how closure rewrites import/export statements. Furthermore, the .d.ts file it generated was extremely crufty, and contained stuff like this:

declare module 'goog:module$_closure_tmp$packages$mdc_icon_toggle$index' {
  import alias = ಠ_ಠ.clutz.module$_closure_tmp$packages$mdc_icon_toggle$index;
  export = alias;
}
declare namespace ಠ_ಠ.clutz.module$_closure_tmp$packages$mdc_ripple$adapter {
}
declare module 'goog:module$_closure_tmp$packages$mdc_ripple$adapter' {
  import alias = ಠ_ಠ.clutz.module$_closure_tmp$packages$mdc_ripple$adapter;
  export = alias;
}
declare namespace ಠ_ಠ.clutz.module$_closure_tmp$packages$mdc_ripple$constants {
}
declare module 'goog:module$_closure_tmp$packages$mdc_ripple$constants' {
  import alias = ಠ_ಠ.clutz.module$_closure_tmp$packages$mdc_ripple$constants;
  export = alias;
}
declare namespace ಠ_ಠ.clutz.module$_closure_tmp$packages$mdc_ripple$foundation {
}

Along with a bunch of other cruft that did not look like it belonged in a public type declaration file. From this, I surmised that Clutz mainly exists to translate type declarations from legacy closure libraries using the legacy goog.* module system over to Typescript, but not for ES2015 code.

This leaves us with two solutions:

  1. Manually write the typings ourself. Error-prone and time-consuming
  2. Get the community to help out and write the typings. While this is feasible, it seems like an unfair burden for the community.
  3. Come up with a sound way to translate between our closurized code a type declaration file. I have a POC script for this that converts the MDCIconToggleAdapter record type into a typescript interface:
node scripts/lib/record-to-ts-interface.js

declare module '@material/icon-toggle' {
  export interface MDCIconToggleAdapter {
    addClass(className: string);
    removeClass(className: string);
    registerInteractionHandler(type: string, handler: Function);
    deregisterInteractionHandler(type: string, handler: Function);
    setText(text: string);
    getTabIndex(): number;
    setTabIndex(tabIndex: number);
    getAttr(name: string): string;
    setAttr(name: string, value: string);
    rmAttr(name: string);
    notifyChange(evtData: {isOn: boolean});
  }
}

The POC code uses babylon and doctrine to parse our JSDoc'd ES2015 code, and then uses very simple and hacky codegen to emit the proper type declaration. While this is simply a POC, I believe that with a bit more work we could build a robust translation system for MDC-Web and support Typescript as a first-class citizen within our codebase.

Conclusion

There are still some rough edges to iron out, but nothing that precludes us from having the best of all worlds:

  • Annotating our JS for advanced optimizations by the closure compiler
  • Using babel + webpack to provide distributions to our external users via npm
  • Using closure compiler to provide idiomatic, fully-compilable code to google products and other applications that require this
  • Using our closurized code to create type declaration files for the TS community.

Next Steps

  • Discuss all research with the team
  • Sketch out a CI infrastructure that would allow us to begin incrementally building and testing our modules against the closure compiler
  • Drafting up issues surrounding annotating our code for closure
  • Conduct further investigation into stripping closure-specific code out of our distributions
  • Conduct further investigation into generating type declaration files from closurized code

cc @material-components/mdc-web

@traviskaufman
Copy link
Contributor Author

@Elemecca
Copy link
Contributor

The linked milestone in the last comment has disappeared. Is there a new public location that's tracking the status of the Closure/TypeScript project?

@sgammon
Copy link

sgammon commented May 18, 2019

hey @traviskaufman - i know it has been ages but what's the status on this? since that milestone is now gone, is MDC buildable via the Closure toolchain?

@sgammon
Copy link

sgammon commented Nov 13, 2020

yoooooo @traviskaufman lol

@sgammon
Copy link

sgammon commented Nov 13, 2020

this is such a sad issue thread tbh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants