chore: replace JS for TS #4435

moog16 · 2019-02-19T21:20:20Z

related #4225

I'm fairly certain I found all instances of .js that needed to be replaced to .ts. I ignored all demos/tests since we aren't rewriting those just yet.

TODO:

revisit the check-imports.js script

codecov-io · 2019-02-19T21:29:11Z

Codecov Report

Merging #4435 into feat/typescript will increase coverage by <.01%.
The diff coverage is n/a.

@@                 Coverage Diff                 @@
##           feat/typescript    #4435      +/-   ##
===================================================
+ Coverage            99.05%   99.05%   +<.01%     
===================================================
  Files                  130      130              
  Lines                 6238     6239       +1     
  Branches               813      813              
===================================================
+ Hits                  6179     6180       +1     
  Misses                  58       58              
  Partials                 1        1

Impacted Files	Coverage Δ
packages/mdc-radio/component.ts	`86.95% <0%> (ø)`	⬆️
packages/mdc-menu-surface/foundation.ts	`100% <0%> (ø)`	⬆️
packages/mdc-checkbox/component.ts	`95.74% <0%> (ø)`	⬆️
packages/mdc-auto-init/index.ts	`95.45% <0%> (+0.21%)`	⬆️

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 c6b609c...7cd0007. Read the comment docs.

kfranqueiro

After starting to look through this PR, I think we should leave the Getting Started guide for a separate PR, if we touch it at all. The most important question is, will it continue to work as-is? (Since that's the entire point of us continuing to build the ESM where the ES2015+ source files used to reside.)

I'm concerned that the Getting Started guide becomes even more overwhelming and complicated than it already is, and if someone's interested in writing TS themselves, I feel like there should be separate resources for that beyond the scope of our repository.

kfranqueiro · 2019-02-20T19:59:47Z

docs/closure-compiler.md

@@ -1,179 +0,0 @@
-<!--docs:


Are you sure we should remove this file...?

I had already updated this in an attempt to filter it down to things that still seemed relevant.

@acdvorak does this file still have merit for purposes of avoiding breakages when we mirror code internally? For that matter is there any stuff missing here that should be noted? (Or is anything that was notable in here present in best_practices.md?)

Well if we want to keep this as a best practices for writing foundations/adapter/components with TypeScript that makes sense. But a lot of this seems to be covered in the authoring-components.md. What do you think?

docs/authoring-components.md

docs/code/architecture.md

docs/contribute/code_review.md

kfranqueiro · 2019-02-20T20:05:28Z

docs/getting-started.md

@@ -149,7 +149,7 @@ And open http://localhost:8080 in a browser. You should see a blue “Hello Worl
 Now that you have webpack configured to compile Sass into CSS, let's include the Sass files for the Material Design button. First install the Node dependency:

 ```
-npm install --save-dev @material/button


Don't remove --save-dev. I'm pretty sure I stopped someone from doing this before (or had them revert it).

Why are we telling people to add their non-build step deps to devDeps? dependencies are modules which are also required at runtime.

kfranqueiro · 2019-02-20T20:09:19Z

docs/getting-started.md

 You can install all of them by running this command:

 ```
-npm install --save-dev babel-core@6 babel-loader@7 babel-preset-es2015 babel-plugin-transform-object-assign
+npm install --save-dev babel-core@6 babel-loader@7 babel-preset-es2015 babel-plugin-transform-object-assign [email protected]


Should typescript also be installed locally? It's not in this list...

Also, babel-plugin-transform-object-assign should no longer be needed (we should test that - I'm pretty sure all instances of Object.assign should no longer exist on the feat/typescript branch)

Hmm...ya i suppose it should. Below I also recommend installing globally, but it should be included in their deps in package.json

docs/getting-started.md

mdc-web-bot · 2019-02-22T18:15:11Z

All 624 screenshot tests passed for commit efdd684 vs. feat/typescript! 💯🎉

mdc-web-bot · 2019-02-22T18:17:27Z

All 624 screenshot tests passed for commit 7d9fed4 vs. feat/typescript! 💯🎉

mdc-web-bot · 2019-02-22T18:20:04Z

All 624 screenshot tests passed for commit c5206d7 vs. feat/typescript! 💯🎉

mdc-web-bot · 2019-02-22T18:21:58Z

All 624 screenshot tests passed for commit 9963010 vs. feat/typescript! 💯🎉

mdc-web-bot · 2019-02-22T19:40:00Z

All 624 screenshot tests passed for commit 380bdd0 vs. feat/typescript! 💯🎉

acdvorak · 2019-02-25T02:00:51Z

docs/importing-js.md

 Note that in this case, you must ensure your build toolchain is configured to process and transpile MDC Web's modules
 as well as your own. You will also need to include babel's
 [`transform-object-assign`](https://www.npmjs.com/package/babel-plugin-transform-object-assign) plugin for IE 11 support.

 See the [Getting Started guide](getting-started.md) for more details on setting up an environment.

+#### TypeScript
+
+If you are using TypeScript, MDC Web's packages also include `.d.ts` files for your consumption. Most of the time you shouldn't need to reference these, as the TypeScript compiler should automatically pick them up. MDC Web has also defined the `types` property found in `package.json`, for your convinence. There is a bundled `.d.ts` file found under the `dist` directory that maps to the respective UMD module. There are corresponding `.d.ts` files for each foundation/component/adapter/etc. within the package.


Spelling: convinence. Also, what does "for your convenience" mean? Why is the types property important; what does it do?

I read this doc again: https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html

It reads, "Also note that if your main declaration file is named index.d.ts and lives at the root of the package (next to index.js) you do not need to mark the "types" property, though it is advisable to do so."

I've been reading this incorrectly, and its actually required in our case. I'll update the wording.

docs/importing-js.md

docs/contribute/code_review.md

docs/code/architecture.md

docs/authoring-components.md

acdvorak · 2019-02-25T02:15:14Z

docs/authoring-components.md

-      setToggleColorTextContent: (/* textContent: string */) => {},
-      registerInteractionHandler: (/* type: string, handler: EventListener */) => {},
-      deregisterInteractionHandler: (/* type: string, handler: EventListener */) => {}
+      setAttr: (/* attr: string, value: string */) => undefined,


Replace Closure Doc comments with TypeScript

I replaced it with:

getAttr: (attr: string) => '', setAttr: (attr: string, value: string) => undefined, addClass: (className: string) => undefined, removeClass: (className: string) => undefined, setToggleColorTextContent: (textContent: string) => undefined, registerInteractionHandler: (type: string, handler: EventListener) => undefined, deregisterInteractionHandler: (type: string, handler: EventListener) => undefined

Although in our actual implementation we don't have unused arguments. Should I just remove them to look like:

getAttr: () => '', setAttr: () => undefined, addClass: () => undefined, removeClass: () => undefined, setToggleColorTextContent: () => undefined, registerInteractionHandler: () => undefined, deregisterInteractionHandler: () => undefined,

I would probably leave them to make it clear that they exist, even if we're not using them.

docs/authoring-components.md

mdc-web-bot · 2019-02-25T19:51:21Z

All 624 screenshot tests passed for commit aa80f74 vs. feat/typescript! 💯🎉

acdvorak · 2019-02-26T19:57:04Z

We can probably update check-imports.js in PR #4449.

acdvorak

A few small wording suggestions, but otherwise LGTM!

acdvorak · 2019-02-26T22:42:04Z

docs/authoring-components.md

-      ├── constants.js # The component's cssClasses/strings/numbers constants
-      ├── foundation.js # The component's foundation class
-      ├── index.js # The file that contains the vanilla component class, as well as exports the foundation
+      ├── adapter.ts # The adapter interface implemented by the component


# Adapter interface implemented by framework wrappers and the vanilla component

Were some of Andy's comments not resolved?

I'm just about to fix these. I didn't see this yesterday.

acdvorak · 2019-02-26T22:43:27Z

docs/authoring-components.md

-      ├── foundation.js # The component's foundation class
-      ├── index.js # The file that contains the vanilla component class, as well as exports the foundation
+      ├── adapter.ts # The adapter interface implemented by the component
+      ├── foundation.ts # The component's foundation class


# Framework-agnostic business logic used by wrapper libraries and the vanilla component

acdvorak · 2019-02-26T22:44:48Z

docs/authoring-components.md

@@ -478,9 +474,11 @@ A typical component within our codebase looks like so:
 packages
  ├── mdc-component
      ├── README.md # The component's README


# Usage instructions and API documentation

acdvorak · 2019-02-26T22:48:51Z

docs/authoring-components.md

+      ├── foundation.ts # The component's foundation class
+      ├── component.ts # The file that contains the vanilla component class
+      ├── constants.ts # The component's cssClasses/strings/numbers constants
+      ├── index.ts # The file that exports the source files of the package (foundation, component, adapter, constants, etc.)
      ├── mdc-component.scss # The main source file for the component's CSS


Let's add line items for types.ts and util.ts too. Be sure to mention that they're both optional.

All non-component-related types and interfaces not covered by other files should go in types.ts.

util.ts is for framework-agnostic helper functions (e.g., feature detection).

acdvorak · 2019-02-26T22:49:54Z

docs/authoring-components.md

-      ├── index.js # The file that contains the vanilla component class, as well as exports the foundation
+      ├── adapter.ts # The adapter interface implemented by the component
+      ├── foundation.ts # The component's foundation class
+      ├── component.ts # The file that contains the vanilla component class


# Vanilla adapter implementation for clients who aren't using a wrapper framework like React

acdvorak · 2019-02-26T22:51:42Z

docs/authoring-components.md

+      ├── adapter.ts # The adapter interface implemented by the component
+      ├── foundation.ts # The component's foundation class
+      ├── component.ts # The file that contains the vanilla component class
+      ├── constants.ts # The component's cssClasses/strings/numbers constants


# Constant values used by one or more files in the package (e.g., cssClasses, strings, numbers)

acdvorak · 2019-02-26T22:52:54Z

docs/authoring-components.md

+      ├── foundation.ts # The component's foundation class
+      ├── component.ts # The file that contains the vanilla component class
+      ├── constants.ts # The component's cssClasses/strings/numbers constants
+      ├── index.ts # The file that exports the source files of the package (foundation, component, adapter, constants, etc.)


# Re-exports types from other files in the package (adapter, foundation, component, util, etc.)

acdvorak · 2019-02-26T23:02:55Z

docs/importing-js.md

@@ -26,12 +26,20 @@ of your built assets, you will want to explicitly reference the package's `index
 import {MDCFoo, MDCFooFoundation} from '@material/foo/index';
 ```

+Certain build tools will often pick up on the use of `package.json`'s `module` property, which points to the ES2015+ `index.js` source code. If you are using [Webpack](https://webpack.js.org/) or [Rollup](https://rollupjs.org/guide/en), then you will not need to reference the `index.js` file, and can continue to use the `@material/foo` import paths.


Certain build tools will detect the `module` property in `package.json`, which points to ES2015+ source code in an `index.js` file. If you're using [Webpack](https://webpack.js.org/) or [Rollup](https://rollupjs.org/guide/en), you do not need to reference `/index` directly, and can continue to use the shorter `@material/foo` import path syntax.

(Reformat the above into one line)

It looks like this comment hasn't been applied.

Furthermore, index.js isn't ES2015+ source anymore. It's ES5 source in an ES module.

How would you phrase "ES5 source in an ES module" in context?

The 2 index.js files of the npm package are:

dist/index.js (ES5) -- UMD index.js (ES2015+) -- ESModule

Isn't what Andy wrote correct?

The distinction is that prior to the TS conversion, *.js was ES modules with ES2015+ syntax, thus requiring babel and additionally the object assign polyfill. After the TS conversion, *.js are ES modules with ES5 syntax. I believe that means you can get away with just webpack and not babel at all?

In terms of phrasing, something like: "which points to an ES module otherwise containing only ES5 syntax". We could link something to Rollup's pkg.module definition at https://github.com/rollup/rollup/wiki/pkg.module if we want.

@kfranqueiro I just checked by running npm run build:esmodules, so I do think you're correct.

Also I just noticed that we don't use Object.assign anywhere in our packages/ directory. I do not think you need babel for either of our UMD or ESM packages.

RE your last point, that's what I mean. Previously we were relying on the object assign polyfill to generate our UMD (you were never expected to need babel to consume those). Now we don't need to use that polyfill for our build process, and neither does anyone else using us within their own.

In getting-started.md there is this dep:

- [babel-plugin-transform-object-assign](http://npmjs.org/package/babel-plugin-transform-object-assign), for IE 11 support

I am going to remove it

kfranqueiro · 2019-02-26T23:34:01Z

docs/authoring-components.md

 class RedblueToggle extends MDCComponent {
+  initialize() {
+    this.root.addEventListener('click', this.foundation_.handleClick);


These can use this.listen / this.unlisten here now that the class extends MDCComponent

kfranqueiro · 2019-02-26T23:34:55Z

docs/authoring-components.md

-      ├── constants.js # The component's cssClasses/strings/numbers constants
-      ├── foundation.js # The component's foundation class
-      ├── index.js # The file that contains the vanilla component class, as well as exports the foundation
+      ├── adapter.ts # The adapter interface implemented by the component


Were some of Andy's comments not resolved?

kfranqueiro · 2019-02-26T23:37:58Z

docs/closure-compiler.md

-The following guidelines outline the general conventions for writing closure-friendly code for MDC Web.
-This section should contain most - if not all - of what you need to get up and running writing components in our codebase.
-
-### All `import` statements must _not_ use re-exported modules.


This should effectively be moved to best_practices.md in the form of a heading explaining that in the context of component packages, only the index and component files are allowed to reference index, because wrappers shouldn't need to pull in index or component

Is this line still accurate?

This is an unfortunate side-effect of how closure's module naming mechanism works.

kfranqueiro · 2019-02-26T23:38:53Z

docs/closure-compiler.md

-
-Using this convention allows us to write tooling around handling these expressions, such as lint rule exceptions.
-
-### For key/value maps, use index signatures where possible


This should be moved to best_practices.md; I added this specifically for TypeScript

Re both closure-compiler comments: I ended up adding this entire block:

#### All `import` statements must _not_ use re-exported modules Only the `index.ts` or `component.ts` files are allowed to reference from other component packages' `index.ts`. This is because wrapping libraries only use `foundation` and `adapter`, so we should decouple the `component`. ```ts // BAD import {MDCFoundation} from '@material/base'; // GOOD import {MDCFoundation} from '@material/base/foundation';

All adapters must be defined as interfaces

Each adapter must be defined within an adapter.ts file in the component's package directory.
All methods should contain a summary of what they should do. This summary should be
copied over to the adapter API documentation in our README. This will facilitate future endeavors
to potentially automate the generation of our adapter API docs. Note that this replaces the
inline comments present in the methods within defaultAdapter.

// adapter.ts export interface MDCComponentAdapter { /** * Adds a class to the root element. */ addClass(className: string): void; /** * Removes a class from the root element. */ removeClass(className: string): void; }

Foundation classes must extend MDCFoundation

Foundations must extend MDCFoundation parameterized by their respective adapter. The
defaultAdapter must return an object with the correct adapter shape.

// foundation.ts import {MDCFoundation} from '@material/base/foundation'; import MDCComponentAdapter from './adapter'; export class MDCComponentFoundation extends MDCFoundation<MDCComponentAdapter> { static get defaultAdapter(): MDCComponentAdapter { return { addClass: (className: string) => undefined, removeClass: (className: string) => undefined, }; } }

Component classes must extend MDCComponent

Components must extend MDCComponent parameterized by their respective foundation.

// index.ts import {MDCComponent} from '@material/base/component'; import MDCComponentFoundation from './foundation'; export class MDCAwesomeComponent extends MDCComponent<MDCComponentFoundation> { getDefaultFoundation(): MDCComponentFoundation { return new MDCComponentFoundation({ addClass: (className: string) => this.root_.classList.add(className), removeClass: (className: string) => this.root_.classList.remove(className), }); } }

For key/value maps, use index signatures where possible

Index signatures are useful for homogeneous maps of key/value pairs, while interfaces are useful for specific object signatures.
Both are more specific than just typing something as object, and are thus preferable for their respective use cases.

static get strings(): {[key: string]: string} { ... }

kfranqueiro · 2019-02-26T23:40:15Z

docs/importing-js.md

@@ -26,12 +26,20 @@ of your built assets, you will want to explicitly reference the package's `index
 import {MDCFoo, MDCFooFoundation} from '@material/foo/index';
 ```

+Certain build tools will often pick up on the use of `package.json`'s `module` property, which points to the ES2015+ `index.js` source code. If you are using [Webpack](https://webpack.js.org/) or [Rollup](https://rollupjs.org/guide/en), then you will not need to reference the `index.js` file, and can continue to use the `@material/foo` import paths.


It looks like this comment hasn't been applied.

Furthermore, index.js isn't ES2015+ source anymore. It's ES5 source in an ES module.

mdc-web-bot · 2019-02-27T19:21:12Z

All 624 screenshot tests passed for commit 42e349b vs. feat/typescript! 💯🎉

mdc-web-bot · 2019-02-27T19:26:36Z

All 624 screenshot tests passed for commit 160fd92 vs. feat/typescript! 💯🎉

mdc-web-bot · 2019-02-27T19:35:40Z

All 624 screenshot tests passed for commit 0790186 vs. feat/typescript! 💯🎉

mdc-web-bot · 2019-03-01T18:06:54Z

All 624 screenshot tests passed for commit 618d556 vs. feat/typescript! 💯🎉

mdc-web-bot · 2019-03-01T19:31:04Z

All 624 screenshot tests passed for commit 72ce98f vs. feat/typescript! 💯🎉

mdc-web-bot · 2019-03-01T21:00:51Z

All 624 screenshot tests passed for commit 7cd0007 vs. feat/typescript! 💯🎉

Matt Goo added 7 commits February 14, 2019 14:15

docs: update docs for typescript

90f3a1e

Merge branch 'feat/typescript' into docs/update-for-ts

a06c004

docs: update authoring components

1fbeb38

Update architecture.md

7eff056

docs: add ammendment to importingjs

11f01b6

WIP: replace js for ts

6b1f2ef

docs: update getting started

de3ef00

googlebot added the cla: yes label Feb 19, 2019

kfranqueiro mentioned this pull request Feb 19, 2019

Convert packages to TypeScript #4225

Closed

45 tasks

fix: remove todo from check-pkg-for-release

32f1783

kfranqueiro suggested changes Feb 20, 2019

View reviewed changes

Matt Goo added 7 commits February 21, 2019 10:45

Update getting-started.md

3eba222

Update getting-started.md

949d719

Update importing-js.md

5b02334

Merge branch 'feat/typescript' into chore/replace-js-for-ts

efdd684

fix: revert gettingstarted

7d9fed4

Update getting-started.md

c5206d7

fix: typos

9963010

fix: revert change

380bdd0

acdvorak suggested changes Feb 25, 2019

View reviewed changes

fix: PR review comments

aa80f74

acdvorak approved these changes Feb 26, 2019

View reviewed changes

kfranqueiro suggested changes Feb 26, 2019

View reviewed changes

Matt Goo added 4 commits February 27, 2019 09:58

Merge branch 'feat/typescript' into chore/replace-js-for-ts

312ca07

chore: fix PR comments on wording

42e349b

docs: remove babel-object-assign

160fd92

docs: remove object.assign for spread op

0790186

Matt Goo and others added 2 commits February 27, 2019 12:47

fix: remove section of bestpractices

64cb8b6

WIP fixes

618d556

docs: replace es2015 to esmodules

72ce98f

kfranqueiro approved these changes Mar 1, 2019

View reviewed changes

WIP Ripple README syntax fixes

7cd0007

moog16 mentioned this pull request Mar 1, 2019

chore: Replace JS with TS #4472

Merged

moog16 closed this Mar 1, 2019

moog16 deleted the chore/replace-js-for-ts branch March 1, 2019 22:41


		Using this convention allows us to write tooling around handling these expressions, such as lint rule exceptions.

		### For key/value maps, use index signatures where possible

chore: replace JS for TS #4435

chore: replace JS for TS #4435

Uh oh!

Conversation

moog16 commented Feb 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Feb 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kfranqueiro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mdc-web-bot commented Feb 22, 2019

Uh oh!

mdc-web-bot commented Feb 22, 2019

Uh oh!

mdc-web-bot commented Feb 22, 2019

Uh oh!

mdc-web-bot commented Feb 22, 2019

Uh oh!

mdc-web-bot commented Feb 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mdc-web-bot commented Feb 25, 2019

Uh oh!

acdvorak commented Feb 26, 2019

Uh oh!

acdvorak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

moog16 commented Feb 19, 2019 •

edited

Loading

codecov-io commented Feb 19, 2019 •

edited

Loading