-
Notifications
You must be signed in to change notification settings - Fork 519
Update all projects to TypeScript 2.0 - remove typings - etc #362
Changes from 4 commits
7618fd1
1a0be69
935b2ac
60f8902
cfb4b84
63e030d
080e3e6
b6607cf
40ac5b2
de5bb05
5c2741d
44c30ad
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,8 @@ | ||
import './css/site.css'; | ||
import * as ko from 'knockout'; | ||
import { createHistory } from 'history'; | ||
import './webpack-component-loader'; | ||
import AppRootComponent from './components/app-root/app-root'; | ||
var createHistory = require('history').createBrowserHistory | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this change deliberate? Could you clarify why it would be needed (hopefully it isn't), and if it is, change the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Be back shortly and i'll talk to you about this one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So it needs to be updated because when the previous There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems to be connected to tsconfig.json "target": "es5". Switching to es6 results in an error with crossroads. |
||
|
||
// Load and register the <app-root> component | ||
ko.components.register('app-root', AppRootComponent); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ import navMenu from '../nav-menu/nav-menu'; | |
|
||
// Declare the client-side routing configuration | ||
const routes: Route[] = [ | ||
{ url: '', params: { page: 'home-page' } }, | ||
{ url: '/', params: { page: 'home-page' } }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you clarify why this change is needed? If it is (hopefully not), can you make the other lines consistent with it too (i.e., URLs also start with a slash, and make the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was an attempt to fix a knockout issue, I can revert this change! |
||
{ url: 'counter', params: { page: 'counter-example' } }, | ||
{ url: 'fetch-data', params: { page: 'fetch-data' } } | ||
]; | ||
|
@@ -13,7 +13,7 @@ class AppRootViewModel { | |
public route: KnockoutObservable<Route>; | ||
private _router: Router; | ||
|
||
constructor(params: { history: HistoryModule.History }) { | ||
constructor(params: { history: History.History }) { | ||
// Activate the client-side router | ||
this._router = new Router(params.history, routes) | ||
this.route = this._router.currentRoute; | ||
|
@@ -40,4 +40,4 @@ class AppRootViewModel { | |
} | ||
} | ||
|
||
export default { viewModel: AppRootViewModel, template: require('./app-root.html') }; | ||
export default { viewModel: AppRootViewModel, componentName: 'Home', template: require('./app-root.html') }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pretty sure this shouldn't be added. What's the intention here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might of been accidental, Knockout is giving me a minor :
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds like the URL isn't matching any route, so There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So when a refresh is done it doesn't load that routed content, but when the link is clicked, it works fine. Any idea what could be causing this? |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,8 +3,13 @@ | |
"moduleResolution": "node", | ||
"target": "es5", | ||
"sourceMap": true, | ||
"skipDefaultLibCheck": true | ||
"skipDefaultLibCheck": true, | ||
"typeRoots": [ "node_modules/@types" ], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the |
||
"types": ["core-js", "crossroads", "history", "knockout", | ||
"requirejs", "signals", "whatwg-fetch" ] | ||
}, | ||
"compileOnSave": false, | ||
"buildOnSave": false, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are these two lines really needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They are just helpful depending on certain IDE's to disable automatic There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are they only in one template and not all? (As far as I know the default value of buildOnSave is already false.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have removed them |
||
"exclude": [ | ||
"bin", | ||
"node_modules" | ||
|
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep renaming separate from this PR. Can you revert the names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was breaking since webpack (#363) is looking for a file by that name, want me to change it there instead? @SteveSandersonMS