Update all projects to TypeScript 2.0 - remove typings - etc #362

MarkPieszak · 2016-10-05T16:16:21Z

All projects up & running now with TS2.0.

* Upgrade Angular2 & TS2 ReactRedux template * ReactSpa & WebAppBasic & Knockout to 2.0 * Update Store * Fix Knockout

colltoaction · 2016-10-05T16:20:21Z

chokidar/package.json

+        "spec": ">=1.0.0 <2.0.0",
+        "type": "range"
+      },
+      "C:\\Users\\mark.pieszak\\Documents\\GitHub\\JavaScriptServices\\templates\\KnockoutSpa\\node_modules\\watchpack"


This is probably not right (your path). But what is chokidar doing here anyway?

I'm not sure removing it now... just realized that too

Removed it now!

SteveSandersonMS · 2016-10-05T16:25:31Z

Thanks for the update @MarkPieszak!

There seems to be some unrelated stuff in the PR that was perhaps added by mistake. For example, the chokidar source code for some reason, and renaming boot-client.ts to main-client.ts etc.

Could you remove the unrelated changes from this PR so that it's just the updates for TS 2.0?

SteveSandersonMS · 2016-10-05T16:29:39Z

templates/KnockoutSpa/ClientApp/boot.ts

 import './webpack-component-loader';
 import AppRootComponent from './components/app-root/app-root';
+var createHistory = require('history').createBrowserHistory


Is this change deliberate? Could you clarify why it would be needed (hopefully it isn't), and if it is, change the var to const and add the semicolon at the end of the line?

Be back shortly and i'll talk to you about this one.

So it needs to be updated because when the previous import {} way was done, with the new History, it doesn't call the correct methods and ends up erroring that "createBrowserHistory" doesn't exist. I've tried the other ways suggested on the packages readme (https://github.com/mjackson/history) but only this one works). Updating it to const now.

It seems to be connected to tsconfig.json "target": "es5". Switching to es6 results in an error with crossroads.

SteveSandersonMS · 2016-10-05T16:30:14Z

templates/KnockoutSpa/ClientApp/components/app-root/app-root.ts

@@ -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') };


Pretty sure this shouldn't be added. What's the intention here?

This might of been accidental, Knockout is giving me a minor :
Any idea what that might be? @SteveSandersonMS

Uncaught Error: Unable to process binding "component: function (){return { name:route().page,params:route} }" Message: No component name specified```

Sounds like the URL isn't matching any route, so route() is an empty object, so route().page is undefined, so you're returning nothing for the name of the component.

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?

SteveSandersonMS · 2016-10-05T16:31:09Z

templates/KnockoutSpa/ClientApp/components/app-root/app-root.ts

@@ -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' } },


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 params line up vertically).

This was an attempt to fix a knockout issue, I can revert this change!
reverted

SteveSandersonMS · 2016-10-05T16:32:32Z

templates/KnockoutSpa/tsconfig.json

  },
+  "compileOnSave": false,
+  "buildOnSave": false,


Are these two lines really needed?

They are just helpful depending on certain IDE's to disable automatic tsc compilation / etc.
Do you want me to remove them? Just thought they were helpful to ensure webpack handles everything.

Why are they only in one template and not all? (As far as I know the default value of buildOnSave is already false.)

Have removed them

SteveSandersonMS · 2016-10-05T16:32:45Z

templates/KnockoutSpa/tsconfig.json

@@ -3,8 +3,13 @@
    "moduleResolution": "node",
    "target": "es5",
    "sourceMap": true,
-    "skipDefaultLibCheck": true
+    "skipDefaultLibCheck": true,
+    "typeRoots": [ "node_modules/@types" ],


Is this needed?

Removed the typeRoots part: (node_modules/@types) is default.
The part where types:[] is set is needed otherwise it won't make them necessary during compilation.

SteveSandersonMS · 2016-10-05T16:34:00Z

templates/ReactReduxSpa/ClientApp/configureStore.ts

-import { routerReducer } from 'react-router-redux';
-import * as Store from './store';
-import { typedToPlain } from 'redux-typed';
+import * as Redux from "redux";


For consistency, can we go back to single-quotes for all lines in this file?

SteveSandersonMS · 2016-10-05T16:34:36Z

templates/ReactReduxSpa/ClientApp/configureStore.ts

+import { routerReducer } from "react-router-redux";
+import * as Store from "./store";
+import { typedToPlain } from "redux-typed";
+


There's already a blank line below this - don't need another :)

Haha sounds good, be right back (grabbing lunch) and I'll fix the above comments 👍

SteveSandersonMS · 2016-10-05T16:35:43Z

templates/ReactReduxSpa/tsconfig.json

-    "skipDefaultLibCheck": true
+    "skipDefaultLibCheck": true,
+    "lib": ["es6", "dom"],
+    "typeRoots": [ "node_modules/@types" ],


Is this needed?

typeRoots can be removed since it's the default. Done

SteveSandersonMS · 2016-10-05T16:39:56Z

templates/Angular2Spa/webpack.config.js

@@ -41,7 +41,7 @@ var clientBundleConfig = merge(sharedConfig, {

 // Configuration for server-side (prerendering) bundle suitable for running in Node
 var serverBundleConfig = merge(sharedConfig, {
-    entry: { 'main-server': './ClientApp/boot-server.ts' },
+    entry: { 'main-server': './ClientApp/main-server.ts' },


Let's keep renaming separate from this PR. Can you revert the names?

It was breaking since webpack (#363) is looking for a file by that name, want me to change it there instead? @SteveSandersonMS

stephtr · 2016-10-05T17:03:20Z

templates/ReactReduxSpa/tsconfig.json

+    "lib": ["es6", "dom"],
+    "typeRoots": [ "node_modules/@types" ],
+    "types": [ "react", "react-dom", "react-redux", "react-router", "react-router-redux",
+     "redux", "redux-thunk", "source-map", "uglify-js", "webpack", "webpack/webpack-env",


Even though it is not yet at npm: webpack-env definition has been outsourced into an additional package (DefinitelyTyped #11684).

Ah so soon we want to specifically add it?

Theoretically it should be already online. I filed an issue at types-publisher #159.

@types/webpack-env is online now.

great, i'll replace webpack/webpack-env in here with that @types/webpack-env. Thanks @stephtr !

stephtr · 2016-10-05T17:10:39Z

templates/ReactReduxSpa/ClientApp/configureStore.ts

-    const devToolsExtension = windowIfDefined && windowIfDefined.devToolsExtension; // If devTools is installed, connect to it
+    const windowIfDefined = typeof window === "undefined" ? null : window as any;
+    // If devTools is installed, connect to it
+    const devToolsExtension = windowIfDefined && windowIfDefined.devToolsExtension as () => GenericStoreEnhancer;
    const createStoreWithMiddleware = compose(


Not relevant yet, but I wanted to note it somewhere: This line won't work with the most recent version of redux.d.ts (redux #1936) and maybe have to be changed in the future to compose<Redux.StoreEnhancerStoreCreator<Store.ApplicationState>>(, which doesn't work with the version available now on npm.

It won't work with version 3.5.30? I pinned it to the 3.5.27 for now

I made a pull request at DefinitelyTyped (#11302) including this change.
But that wouldn't make any difference because TypeScript ignores @types/redux/index.d.ts in favor of redux/index.d.ts (definition included within redux npm package). Therefore the next release of redux propably introduces the error.

Just another note on these lines, they could be replaced (like in the redux documentation) by:

const store = createStore( buildRootReducer(Store.reducers), initialState, compose( applyMiddleware(thunk, typedToPlain), devToolsExtension ? devToolsExtension() : f => f ) );

Which also eliminates one typecast and the need for importing Redux.Store.

compose(...) as StoreEnhancer<Store.ApplicationState> would be an option which is compatible with redux 3.6.0 and upcoming versions.

stephtr · 2016-10-05T17:25:27Z

templates/ReactReduxSpa/ClientApp/configureStore.ts

-import * as Store from './store';
-import { typedToPlain } from 'redux-typed';
+import * as Redux from "redux";
+import { createStore, applyMiddleware, compose, combineReducers, GenericStoreEnhancer } from "redux";


Maybe it is better to import { Store as ReduxStore, createStore, ... } from 'redux'; instead of having two times import ... from 'redux';?

Just noticed that, somehow overlooked that line too! :P

MarkPieszak · 2016-10-05T19:36:18Z

@SteveSandersonMS This should be all set now, minus the content not loading right away in the KnockoutSpa template (maybe you know what's causing it?)

Removed the typeRoots, changed createHistry to const
Eliminated double Enters, Double quotes on the one page, etc.

stephtr · 2016-10-05T19:41:50Z

Just for information: The redux-typed/StrongActions.d.ts error is still remaining.

MarkPieszak · 2016-10-05T21:18:11Z

Yes, strangely the error comes from within node_modules itself. Made an issue, but seems no one has responded to it, also doesn't seem to prevent anything from working luckily!

antmdvs · 2016-10-06T02:24:06Z

templates/ReactReduxSpa/tsconfig.json

+    "lib": ["es6", "dom"],
+    "types": [ "react", "react-dom", "react-redux", "react-router", "react-router-redux",
+     "redux", "redux-thunk", "source-map", "uglify-js", "webpack", "webpack/webpack-env",
+      "whatwg-fetch" ]
  },
  "exclude": [


@SteveSandersonMS Should wwwroot be excluded as well? If so, can we sneak that in this PR?

It's up to you guys, let me know I can push it in there!

I defer to @SteveSandersonMS, as I'm not even sure if it's necessary. Just something I noticed.

@antmdvs I'm not sure why wwwroot should be excluded here, but if you think there's something wrong, could you please file a separate issue specifying what problem occurs and what should be done to fix it? Thanks!

stephtr · 2016-10-07T23:04:14Z

As far as I noticed there is a declare var module: any; in ReactReduxSpa/ClientApp/routes.tsx, which isn't needed anymore.

# Conflicts: # templates/Angular2Spa/package.json # templates/KnockoutSpa/package.json # templates/ReactReduxSpa/package.json # templates/ReactSpa/package.json

MarkPieszak · 2016-10-13T15:05:31Z

@SteveSandersonMS Everything has been updated to the latest from master.
The only issue remaining (as far as I've seen) is KnockoutSpa not painting that initial Route if you could take a look?

I didn't want to add angular2 recent upgrades (2.1 & universal 2.1 patches) until we were finished with this one yet. Let me know what you think!

empz · 2016-10-14T20:12:51Z

The StrongActions.d.ts error is easily fixed by setting the dispatch argument type to Dispatch<TState>, but since Redux now includes its own typings, wouldn't be better to leverage them so we stay as close to "official" as possible?
Also, that redux-typed package seems quite unpopular.

MarkPieszak · 2016-10-14T23:51:37Z

Great idea, totally agree, rather it be as official as possible.
@radu-porumb em zeros comment above looks like what we should be doing.

radu-porumb · 2016-10-15T00:28:31Z

@MarkPieszak That's exactly what I did to move things along. But my project is on a CI pipeline which means as soon as it gets to source control the build server pulls it, builds it and fails spectacularly. I could always just throw out redux-typed and copy-paste the code with the <TState> fix into a local file to keep everything strongly typed but I'd like to know if we're likely to get a package update soon before going that route.

SteveSandersonMS · 2016-10-17T10:05:23Z

@stephtr:

As far as I noticed there is a declare var module: any; in ReactReduxSpa/ClientApp/routes.tsx, which isn't needed anymore.

Well spotted! Removed.

Just for information: The redux-typed/StrongActions.d.ts error is still remaining.

Thanks - fixed (in newly-published redux-typed version 1.0.1).

@MarkPieszak:

The only issue remaining (as far as I've seen) is KnockoutSpa not painting that initial Route if you could take a look?

The issue was that the history module had a breaking change between versions 2 and 4 whereby it no longer fires a navigation event on page load. I've amended the code to populate the initial history state. Unfortunately @types/history is massively out-of-date (it still describes the API for history version 2) so there's another cast-to-any to make this compile.

I didn't want to add angular2 recent upgrades (2.1 & universal 2.1 patches) until we were finished with this one yet. Let me know what you think!

Makes sense. I'm merging this PR now so let's look at 2.1 soon :) Thanks.

@emzero:

Redux now includes its own typings, wouldn't be better to leverage them so we stay as close to "official" as possible?

Mostly I agree with you but it's a wider discussion, so let's move this part of the discussion to a separate issue.

SteveSandersonMS · 2016-10-17T10:16:54Z

Merged! Thanks @MarkPieszak for your work on this, and for accounting for many changes while waiting for it to be merged.

MarkPieszak · 2016-10-17T12:28:42Z

Not a problem, glad to be a part of it all :)
Let me know if there's any other random issues you'd like me to look into? @SteveSandersonMS

SteveSandersonMS · 2016-10-17T12:47:38Z

Let me know if there's any other random issues you'd like me to look into

@MarkPieszak The biggest thing for the Angular2Spa template would be if there was some way to get AoT compilation and tree-shaking. But it would have to be pretty much transparent to the developer and not make the webpack config a lot more complex. I don't know if that's possible yet, because last time I checked, ngc didn't have any supported way to work in a Webpack environment.

colltoaction · 2016-10-17T13:02:25Z

It's transparent with @ngtools/webpack. But I have to admit I don't see a
great size improvement with tree shaking. Maybe I'm missing something.

I also mentioned I don't know if it'll work with Universal because I
removed it from my project.

I can share my webpack config later if you want!

On Mon, Oct 17, 2016, 09:47 Steve Sanderson [email protected]
wrote:

Let me know if there's any other random issues you'd like me to look into

@MarkPieszak https://github.com/MarkPieszak The biggest thing for the
Angular2Spa template would be if there was some way to get AoT compilation
and tree-shaking. But it would have to be pretty much transparent to the
developer and not make the webpack config a lot more complex. I don't know
if that's possible yet, because last time I checked, ngc didn't have any
supported way to work in a Webpack environment.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#362 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABeg9DlEG3K9Eobrm09DkpMgc7A00M7eks5q025xgaJpZM4KPAOA
.

MarkPieszak · 2016-10-17T13:14:39Z

Tinchou is right, ngtools/webpack is avaialable, but there's still a lot of things in the works to make everything more stable.

On our end, on the Universal side, we still have a little bit of ground to cover, there has been some work done to get some basic AoT going, but I think it might be a good idea to let things mature a little more before moving it over here as well? (Angular Core itself needs to mature AoT itself in a few areas)
The performance/speed improvements (so far) are incredible, so it would be amazing to get it in here 👍

@SteveSandersonMS But that sounds good to me! I'll start playing around with some things, at least open up a PR sometime so we can begin looking at webpack2's tree-shaking & AoT combination for the template.

colltoaction · 2016-10-17T17:33:11Z

Here's my example configuration https://gist.github.com/tinchou/08d1be519b525899969180bdf7edb67f

MarkPieszak added 3 commits October 5, 2016 09:25

Merge remote-tracking branch 'refs/remotes/aspnet/dev' into dev

7618fd1

Merge remote-tracking branch 'refs/remotes/aspnet/dev' into dev

1a0be69

Update all projects to TypeScript 2.0 - remove typings - etc (#1)

935b2ac

* Upgrade Angular2 & TS2 ReactRedux template * ReactSpa & WebAppBasic & Knockout to 2.0 * Update Store * Fix Knockout

dnfclas added the cla-already-signed label Oct 5, 2016

MarkPieszak mentioned this pull request Oct 5, 2016

chore(typescript): Upgrade to 2.0 & Types #258

Closed

4 tasks

colltoaction reviewed Oct 5, 2016

View reviewed changes

Remove chokidar (accidental node_module exposed)

60f8902

SteveSandersonMS reviewed Oct 5, 2016

View reviewed changes

stephtr reviewed Oct 5, 2016

View reviewed changes

MarkPieszak added 2 commits October 5, 2016 15:27

Updates (const, remove componentName)

cfb4b84

remove last typeRoots

63e030d

antmdvs reviewed Oct 6, 2016

View reviewed changes

MarkPieszak added 3 commits October 6, 2016 08:33

Add @types/webpack-env to ReactReduxSpa

080e3e6

Revert boot/main naming conventions

b6607cf

Upgrade to Angular 2.0.2

40ac5b2

MarkPieszak added 3 commits October 13, 2016 08:01

Merge remote-tracking branch 'refs/remotes/aspnet/dev' into dev

de5bb05

Merge remote-tracking branch 'refs/remotes/aspnet/dev' into dev

5c2741d

# Conflicts: # templates/Angular2Spa/package.json # templates/KnockoutSpa/package.json # templates/ReactReduxSpa/package.json # templates/ReactSpa/package.json

fix knockout package.json merge

44c30ad

SteveSandersonMS closed this Oct 17, 2016

Update all projects to TypeScript 2.0 - remove typings - etc #362

Update all projects to TypeScript 2.0 - remove typings - etc #362

Conversation

MarkPieszak commented Oct 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveSandersonMS commented Oct 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkPieszak Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephtr Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkPieszak Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkPieszak Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

stephtr Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephtr Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

MarkPieszak Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephtr Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

stephtr Oct 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkPieszak commented Oct 5, 2016

stephtr commented Oct 5, 2016

MarkPieszak commented Oct 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephtr commented Oct 7, 2016

MarkPieszak commented Oct 13, 2016

empz commented Oct 14, 2016

MarkPieszak commented Oct 14, 2016

radu-porumb commented Oct 15, 2016

SteveSandersonMS commented Oct 17, 2016

SteveSandersonMS commented Oct 17, 2016

MarkPieszak commented Oct 17, 2016

SteveSandersonMS commented Oct 17, 2016

colltoaction commented Oct 17, 2016

MarkPieszak commented Oct 17, 2016 • edited Loading

colltoaction commented Oct 17, 2016

MarkPieszak Oct 5, 2016 •

edited

Loading

stephtr Oct 5, 2016 •

edited

Loading

MarkPieszak Oct 5, 2016 •

edited

Loading

MarkPieszak Oct 5, 2016 •

edited

Loading

stephtr Oct 5, 2016 •

edited

Loading

stephtr Oct 5, 2016 •

edited

Loading

MarkPieszak Oct 5, 2016 •

edited

Loading

stephtr Oct 6, 2016 •

edited

Loading

stephtr Oct 7, 2016 •

edited

Loading

MarkPieszak commented Oct 17, 2016 •

edited

Loading