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

Convert src/ to TypeScript #68

Merged
merged 25 commits into from
Feb 18, 2017
Merged

Convert src/ to TypeScript #68

merged 25 commits into from
Feb 18, 2017

Conversation

tetsuharuohzeki
Copy link
Contributor

@tetsuharuohzeki tetsuharuohzeki commented Feb 17, 2017

Abstract

  • Breaking Change (possible)
  • From some conversation with @azu on Twitter, I tried to convert codes to TypeScript.
  • This changeset includes some changes for build system.
  • This change only does rewrite codes with TypeScript. To publish d.ts in npm, we need push more patch onto this.

Motivation

  • Increase interoperabitily with static typing world like TypeScript or Flowtype.
  • Erase some inconsistency of API system of this library by static checking types of them.

Detailed Design

Change build system

This introduces 2-phase build system.

  1. At 1st phase, Try type checking under src/ with tsc and emit a plain JS code into /__obj/ temp dir.
    In this phase, this also try to copy all javascript file into /__obj/ to keep the content of /src/.
  2. At 2nd phase, Do down-level transform by babel and emit the transformed one into /lib/.

By this change, we can achive to convert our source code to TypeScript incrementally without at giant single patch file.

Unit tese imports from /lib/

By the above change, we need to load the code to test unit tests for incremental converting without changeing
unit tests.

Convert TypeScript

Convert *.js to *.ts simply.

This change escapes from the problem which is that some dependency does not have any d.ts by dirty hacking.

Add *.d.ts and *.js with same name. TypeScript compiler uses *.d.ts to resolve type checking.
But we use *.js file to load a file actually on the runtime. This dirty hack resolves the problem.

Drawback

  • Drop JSDoc.
    • By this change, user who uses Closure Compiler advanced optimization might face to some troubles.
  • Increase Complexity of build system.

Alternative Approach

  • Using Flowtype instead. This is also possible solution.
    But this tries to use TypeScript by conversations with @azu.

Unresolved Items

@tetsuharuohzeki
Copy link
Contributor Author

r? @azu

@azu
Copy link
Member

azu commented Feb 17, 2017

Thanks.
I'll look it in this weekend.

getState() {
return this._storeGroup.getState();
getState<T>(): T {
return (this._storeGroup as any).getState(); // TODO: remove casting `any`
Copy link
Member

@azu azu Feb 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 I have no idea about this.
Can we remove this casting by some changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought to introduce some new interfaces which is implemented by StoreGroup or Store to fix it.
Perhaps is it not a right way?

Copy link
Member

@azu azu Feb 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, This fix should be later.

@@ -32,14 +34,14 @@ export let defaultStoreName = "<Anonymous-Store>";
* Store class
* @public
*/
export default class Store extends Dispatcher {
abstract class Store extends Dispatcher {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Store is not abstruct in some test case.

const store = new Store();

Is it valid in TypeScript?

Of course, Store is just abstract in most case without testing, I think.
Should we will modify test code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it valid in TypeScript?

"At this time", it's valid in TypeScript because we keep all unit test cases as plain js.
However, of course, we need to add class TestStore extends Store to them if we convert test cases to TypeScript.

Copy link
Member

@azu azu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I've checked this PR.

The first overview is looks good to me.
Thanks for nice work.

@@ -48,6 +50,9 @@ export default class Store extends Dispatcher {
return false
}

name: string;
displayName: string;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, displayName is optional.

* unique id in each UseCase instances.
*/
id: string;
displayName: string;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto.
displayName is optional

@azu azu mentioned this pull request Feb 18, 2017
2 tasks
@azu
Copy link
Member

azu commented Feb 18, 2017

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

Successfully merging this pull request may close these issues.

None yet

2 participants