Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Initial commit for react-tic-tac-toe sample #86

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

liminzhu
Copy link
Member

A typescript-react-webpack sample built following the react-webpack guidelines from typescript handbook.

@liminzhu
Copy link
Member Author

@DanielRosenwasser

@@ -0,0 +1,341 @@
/******/ (function(modules) { // webpackBootstrap
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove the bundle?

}

// make a move
private move(pos: number, val: CellValue, callback?: () => void): void {
Copy link
Member

Choose a reason for hiding this comment

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

You only call this with a callback at one place, so just pass this.aiMove wrapped in an arrow function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in ea3e714

@DanielRosenwasser
Copy link
Member

Hey @liminzhu, looks like we're almost there. The sample should not include its dependencies though. That means no typings directory, and no bundled .d.ts files.

@liminzhu
Copy link
Member Author

liminzhu commented Apr 6, 2016

Thanks for helping the review @DanielRosenwasser! Is there a risk of breaking if the .d.ts files are not included? AFAIK typings would download the latest .d.ts, which some day may be incompatible with the react version specified in package.json, although I could see not including .d.ts in the project being a better practice just like node_modules.

@DanielRosenwasser
Copy link
Member

@liminzhu nope because so long as you've locked onto the dependencies in your typings.json file, they're all versioned. Notice react.d.ts here is locked onto commit f407264835650f5f38d4bb2c515a79e7a835916b.

@liminzhu
Copy link
Member Author

liminzhu commented Apr 7, 2016

@DanielRosenwasser Oh my bad I didn't realize that! Deleted the typings directory.

this.handleGameStateChange(this.state.gameState);
}
if (callback) {
callback.call(this);
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, what I meant by my previous comment is that you don't need to bind this to the callback anyhow, so we should get rid of that.

Copy link
Member

Choose a reason for hiding this comment

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

You can also restore the old aiMove method if you feel it's appropriate.

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

Successfully merging this pull request may close these issues.

2 participants