Skip to content
This repository has been archived by the owner on Apr 8, 2020. It is now read-only.

Update to Angular 4 (phase 1) #930

Closed
wants to merge 6 commits into from
Closed

Update to Angular 4 (phase 1) #930

wants to merge 6 commits into from

Conversation

SteveSandersonMS
Copy link
Member

@SteveSandersonMS SteveSandersonMS commented May 11, 2017

As per this comment, this is a first step towards updating to Angular 4. It's an attempt at the minimal changes needed to get Angular 4 working correctly on both client and server.

I've pushed a prerelease version of generator-aspnetcore-spa to NPM so that people can try this out (details below). I'd like to get a pretty substantial level of feedback that this works properly in all the existing scenarios before we merge this into dev and ship it as the released template package.

How to try it

Install the prerelease version of the Yeoman generator:

npm install -g generator-aspnetcore-spa@next

(having already installed Yeoman using npm install -g yo if you don't already have it)

... and then create a new project:

yo aspnetcore-spa

Please tell us how you get on with it.

Notes

There are a couple of implementation details I'm not yet totally happy with:

  • In boot-server.ts, the error handling is messy. Because Angular fires onStable before it fires onError, I've had to use a nasty setImmediate hack to wait for possible errors before rendering the page. This doesn't feel like it should be the right solution. However, I've looked for examples of people using platformDynamicServer across the web, and it seems like virtually nobody is using it yet, there are no official samples, and the few examples I can find don't even make any attempt at error handling at all. So, @MarkPieszak, is the event ordering a bug in Angular, or is there some other way we're meant to catch errors?
  • The ORIGIN_URL token is just a string right now. It would be better if we had an actual class like RequestContext so you could have component constructor parameters like context: RequestContext without having to use the @Inject decorator at all. But when I tried to do that, it didn't work because the code evaluates in the wrong order, and you have to use the hideous forwardRef (which I don't consider acceptable at all) to work around that. If someone can find a better way to do this, that would be great. The problem might go away on its own if we end up getting an ORIGIN_URL from a third-party package (e.g., Mark's aspnetcore-engine).

@danobri
Copy link
Contributor

danobri commented May 12, 2017

Thanks so much for putting this together! Unfortunately, when I run the install I get the following error:

ERROR in ./ClientApp/boot-server.ts
Module not found: Error: Can't resolve 'core-js/shim' in 'C:\Users\Dan\Documents\Visual Studio 2017\Projects\Angular4\ClientApp'
 @ ./ClientApp/boot-server.ts 3:0-23

I tried running npm install manually in my project folder, but that didn't make any difference. If I try to run the project I get this error:

fail: Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware[0]
An unhandled exception has occurred: Call to Node module failed with error: Prerendering failed because of error: Error: Cannot find module "core-js/shim"

Any idea what the issue is?

@astegmaier
Copy link

I hit the same error as @danobri. It seems that running npm install core-js --save and then running webpack again fixes the problem. I submitted PR #931 to address the issue. @SteveSandersonMS is it that simple, or is there something I'm missing?

@SteveSandersonMS
Copy link
Member Author

@danobri @astegmaier Thanks for pointing that out!

I've applied a fix and have pushed an update to generator-aspnetcore-spa@next. If you spot any other troubles, please let us know!

@epitomz
Copy link

epitomz commented May 12, 2017

Thank you for your work, I just ran the install and everything went good. I'm still experiencing an issue I was trying to get rid of thanks this version in angular 4.
I created a map component with openlayers but it can't be prerendered by the server and I get:
Exception: Call to Node module failed with error: Prerendering failed because of error: ReferenceError: window is not defined

Indeed, openlayers tries to access DOM before it's rendered...

I tried to implement the test isPlatformBrowser(this.platformId) in the onInit method of my component but the issue still occurs. (you can find it here)

The only solution I found is to get rid of the asp-prerender-module="ClientApp/dist/main-server" directive in the Index.cshtml file. As you can guess, it's not really acceptable.

Thanks for your help

@danobri
Copy link
Contributor

danobri commented May 12, 2017

@SteveSandersonMS - I've confirmed the core-js/shim issue is fixed - thanks! Now I can run the project, but not only does HMR not seem to be running, if I stop the project and make a change to one of the html files, and then re-run, I don't see the change. Rebuilding the project doesn't seem to make any difference. I'm using VS2017 if that matters.

@SteveSandersonMS
Copy link
Member Author

@epitomz I don't think your issue is specific to this Angular 4 version of the template. In general, you can't access window during server-side rendering, because there is no window on the server.

@danobri I don't think your issue is specific to this Angular 4 version of the template either. It sounds like you're just not running in development mode.

@danobri
Copy link
Contributor

danobri commented May 12, 2017

@SteveSandersonMS I closed the solution and re-opened, and now everything is working as expected, including HMR.

@MarkPieszak
Copy link
Contributor

MarkPieszak commented May 12, 2017

@SteveSandersonMS Apologies for the delay Steve I meant to email you a week or so ago, but just went through a large cross-country move so I've been a little MIA!

Would it be easier to setup some time on Skype/etc to talk through everything?
I'll shoot you an email, maybe next week we can get together.

In boot-server.ts, the error handling is messy. Because Angular fires onStable before it fires onError, I've had to use a nasty setImmediate hack to wait for possible errors before rendering the page. This doesn't feel like it should be the right solution. However, I've looked for examples of people using platformDynamicServer across the web, and it seems like virtually nobody is using it yet, there are no official samples, and the few examples I can find don't even make any attempt at error handling at all. So, @MarkPieszak, is the event ordering a bug in Angular, or is there some other way we're meant to catch errors?

You could catch the error directly from the subscription, as I'm doing here in the aspnetcore-engine ? You're right though, the timing is a little different now, as isStable looks at more than just the Zone to check for App stability.

Also wondering if the missing filter((isStable: boolean) => isStable) might be needed, we have it in all of our Unit tests for platform-server, never thought about whether it was necessary or not.

The ORIGIN_URL token is just a string right now. It would be better if we had an actual class like RequestContext so you could have component constructor parameters like context: RequestContext without having to use the @Inject decorator at all. But when I tried to do that, it didn't work because the code evaluates in the wrong order, and you have to use the hideous forwardRef (which I don't consider acceptable at all) to work around that. If someone can find a better way to do this, that would be great. The problem might go away on its own if we end up getting an ORIGIN_URL from a third-party package (e.g., Mark's aspnetcore-engine).

Yes ideally you want it to be a new InjectionToken<string>('ORIGIN_URL') (what used to be OpaqueToken). Totally agree on forwardRef, definitely don't want people getting into that habit, that's more of a last resort type of thing! 😄

As for platformServerDynamic, the reason it isn't used is that there are no benefits to doing JIT compilation during server renders, so we only recommended AoT compilation through platformServer. Inside the aspnetcore-engine there is actually a compiler function that can take regular NgModules and compile them async to AoT'd ngModuleFactories, so you can just use the regular platformServer.


Anyway, I'd love to sync with you sometime, so we can craft the aspnetcore-engine & whatever else we need here to help aid ASPNET developers! 👍

bootstrap: sharedConfig.bootstrap,
declarations: sharedConfig.declarations,
imports: [
BrowserModule,
Copy link
Contributor

Choose a reason for hiding this comment

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

You want to make sure you have .withServerTransition({}) as this is how guids (such as those for components and styles) are synced between the server & client.

    BrowserModule.withServerTransition({
        appId: 'my-app-id' // make sure this matches with your Server NgModule
    }),

bootstrap: sharedConfig.bootstrap,
declarations: sharedConfig.declarations,
imports: [
ServerModule,
Copy link
Contributor

Choose a reason for hiding this comment

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

You want to make sure you have BrowserModule with serverTransition here as well (with the same appId)

    BrowserModule.withServerTransition({
        appId: 'my-app-id' // make sure this matches with your Server NgModule
    }),

@@ -1,29 +1,22 @@
import 'angular2-universal-polyfills/browser';
import 'reflect-metadata';
import 'zone.js';
Copy link
Contributor

Choose a reason for hiding this comment

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

You could just have (I believe it's a bit smaller) import 'zone.js/dist/zone-node'; here instead, just an idea!

});
moduleRef.destroy();
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

You could catch errors here in the subscription

@SteveSandersonMS
Copy link
Member Author

You could catch the error directly from the subscription, as I'm doing here in the aspnetcore-engine ?

I tried that, but it didn't work. Try throwing an exception from a component constructor and see if catches the error properly.

Also wondering if the missing filter((isStable: boolean) => isStable) might be needed

I tried that too, but it didn't change anything in this case. Also since I'm listening to onStable rather than the isStable observable, I would expect it to fire only when it is stable, though admittedly I didn't read the underlying implementation.

You want to make sure you have .withServerTransition({})

Yes, we definitely do want to get to that eventually, but for this first phase I'm just trying to preserve existing functionality, which doesn't include any server->client state transition. So hopefully we can add that later.

You could just have (I believe it's a bit smaller) import 'zone.js/dist/zone-node'; here instead, just an idea!

Is that for the server only (based on the name -node)? If so, I don't think the size really makes any difference. But if this works on the client too then that would definitely be good to use.

You could catch errors here in the subscription

Again, that doesn't seem to work as a way of getting for example exceptions thrown in component constructors. Though if you can confirm some way of doing it that does work, please let me know!

import { BrowserModule } from '@angular/platform-browser';
import { FormsModule } from '@angular/forms';
import { HttpModule } from '@angular/http';
import { sharedConfig } from './app.module';
Copy link
Contributor

Choose a reason for hiding this comment

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

It might make more sense to have sharedConfig be an NgModule itself that they import within imports[], since most Apps will have lots of NgModules lazy-loaded etc ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting. How does that then work in terms of overriding the config in each of the server/client app modules? If you can post a code sample we can definitely consider this!

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW I tried to do this, but hit a roadblock. If the shared NgModule includes the routing config (which of course you would want it to), then it has to be able to compile all the templates for all the components. But to be able to compile them, it needs to reference either BrowserModule or ServerModule (otherwise it fails with errors like ngIf is unknown). But it can't reference either of those, because it needs to work with both server and client.

So is there any reasonable way to do this? Please let me know if there is. It doesn't seem like a compiled NgModule can possibly be portable across both server and client, which is an unfortunate limitation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry just seeing this now! @SteveSandersonMS

Did you have boot-client & boot-server using browser-app.module & server-app.module ?

To get rid of the ngIf unknown errors, you just import CommonModule into your base app.module NgModule (https://github.com/MarkPieszak/aspnetcore-angular2-universal/blob/master/Client/app/app.module.ts#L51)

You can see in there I have the app.module importing the Routes, and all the base declarations etc that can be shared. Browser & Server-app.module's both import the app.module and they each declare the bootstrap: [ AppComponent ] themselves. You want to make sure app.module doesn't declare a bootstrap Component.

{ provide: 'ORIGIN_URL', useValue: params.origin }
];

return platformDynamicServer(providers).bootstrapModule(AppModule).then(moduleRef => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The team has stressed to never use dynamicServer as JIT compilation just slows down server renders tremendously as you'd really only want ahead of time compiled ones with platformServer.

Just worried we might have Devs use this phase 1 template (and never update it) and get stuck using a non-ideal bootstrapModule serializer.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point as for where we're ultimately heading, but right now people are already using the equivalent to dynamicServer with their Angular 2 sites. So this pull request is not making things any slower.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great! Just making sure :)

@MarkPieszak
Copy link
Contributor

I tried that, but it didn't work. Try throwing an exception from a component constructor and see if catches the error properly.

You're right, I have it spilling out the errors in the debug console of VSCode etc, but the prerendering Times out. I'll investigate this one further!

Are you getting the same result?

I tried that too, but it didn't change anything in this case. Also since I'm listening to onStable rather than the isStable observable, I would expect it to fire only when it is stable, though admittedly I didn't read the underlying implementation.

Investing this one, I'll get back to you. That's quite strange behavior though, you're right.

Yes, we definitely do want to get to that eventually, but for this first phase I'm just trying to preserve existing functionality, which doesn't include any server->client state transition. So hopefully we can add that later.

Sounds good, it doesn't do too much other than make sure they use the same guid.

Is that for the server only (based on the name -node)? If so, I don't think the size really makes any difference. But if this works on the client too then that would definitely be good to use.

Ah sorry I mean using the -node version for boot-server, not boot-client :)

Again, that doesn't seem to work as a way of getting for example exceptions thrown in component constructors. Though if you can confirm some way of doing it that does work, please let me know!

What kind of exceptions are you throwing, throw Error()?
Or doesNotExist() within a constructor?

@SteveSandersonMS
Copy link
Member Author

Again, that doesn't seem to work as a way of getting for example exceptions thrown in component constructors. Though if you can confirm some way of doing it that does work, please let me know!

What kind of exceptions are you throwing, throw Error()?
Or doesNotExist() within a constructor?

Any unhandled exception in a constructor causes this (so throw new Error() is one example, but also undefined is not a property and so on would have the same effect).

We need to catch and handle all the plausible ways that developers will experience errors during server-side rendering, including:

  • Unhandled exceptions from component constructors
  • DI errors (e.g., can't supply parameters to component constructors)
  • Template rendering errors (e.g., referencing nonexistent properties)

Hopefully there's no special code needed to cover all these cases. The Zone's onError event should, at least I would hope, encapsulate all of them.

@SteveSandersonMS
Copy link
Member Author

This is now merged to dev and will ship in the next version of the templates packages.

@SteveSandersonMS SteveSandersonMS deleted the angular4 branch May 17, 2017 09:15
@michelebombardi
Copy link

@SteveSandersonMS Thank you for you work! When the next version (that includes the Angular 4 support) will be released?

@SteveSandersonMS
Copy link
Member Author

@bm-software It's published to NuGet now. The new version number is 0.9.2. You might have to wait another 10-15 mins before NuGet refreshes and acknowledges that the new package exists.

@MarkPieszak
Copy link
Contributor

Nice!
@SteveSandersonMS Sent you an email, hopefully I have the right email address.
Hoping we can sync together sometime? 👍

@OskarKlintrot
Copy link

Please don't laugh at me but... How do I actually update to the latest template package?

@colltoaction
Copy link

Option 1: go through the commits, and apply changes manually

Option 2:

  1. create a new project using the templates
  2. grab the infrastructure files and paste them to your project
  3. fix things as needed, probably looking at the commits as in Option 1

@OskarKlintrot
Copy link

I have already updated my project to ng4 but I want to update the template package as well so I get the latest templates for dotnet new.

@SteveSandersonMS
Copy link
Member Author

There's probably a cleaner way to upgrade, but what I've been doing is just deleting the .templateengine directory from my home directory. This resets the dotnet new templates back to their default state. Then you can reinstall Microsoft.AspNetCore.SpaTemplates.

If anyone knows what the official way to update dotnet new template packages is, please let us know :)

Maybe it's just as simple as running dotnet new --install Microsoft.AspNetCore.SpaTemplates::* again, but I haven't tried that lately.

@ADefWebserver
Copy link

Maybe it's just as simple as running dotnet new --install Microsoft.AspNetCore.SpaTemplates::* again, but I haven't tried that lately.

That worked for me 👍

@OskarKlintrot
Copy link

Same here, dotnet new --install Microsoft.AspNetCore.SpaTemplates::* worked just fine and I could generate a ng4 project after that so that seems to be the way to do it! Thanks @SteveSandersonMS!

@JohnGalt1717
Copy link

So the issue of passing .net core data to server side pre-rendering appears to have gotten more acute with this release and with @MarkPieszak 's code.

Before it was relatively easy to just pass the values through. But in the new code all of that seems to have been removed from the server pipeline.

The reason why this is critical is environment variables etc. You want to be able to tell your SPA for instance where your API is located in the different environments that it gets published to without having to hack a source file to do it.

An example of how to pass server side values to pre-render (and hopefully the same end result on client side) would be greatly appreciated.

@dennyio
Copy link

dennyio commented May 18, 2017

Thanks! @SteveSandersonMS @MarkPieszak

@andyatwork
Copy link

So the issue of passing .net core data to server side pre-rendering appears to have gotten more acute with this release and with @MarkPieszak 's code.

An example of how to pass server side values to pre-render (and hopefully the same end result on client side) would be greatly appreciated.

Would very much appreciate to see the same.

@JohnGalt1717
Copy link

I'd also love to see data caching so that a page can load on the server side and then pass the cached data to the clientside so that there aren't duplicate calls constantly.

@MarkPieszak
Copy link
Contributor

MarkPieszak commented May 19, 2017

@JohnGalt1717
Apologies I haven't gotten back to you sooner, it's definitely something we want to improve and make even easier!!

Inside my Repo, you pass in whatever custom data you want within TransferData in the HomeController, then you pass it further down with main.server (there should be comments in there explaining the rest).

As for not having Http calls reused you need to either setup a Redux that repopulate the store from the data passed down through the window, or you can use the provided TransferHttp (here). That automatically handles all of that, and doesn't rerun Gets on the client.

Coming soon we are working on adding an easier way to pass data withServerTransition, so that'll help at least from the Angular perspective.

I'll spend some time trying to improve the process, at the moment it's a little difficult 🙈
Apologies for the delay lately, moved cross-country. 🌴

@JohnGalt1717
Copy link

@MarkPieszak Thanks for the info. I've already tried all of that and I can't get the data out that is being set in the HomeController.

I CAN get it out on the client side with window.TRANSFER_CACHE. but I cannot figure out how to get it out on node server side pre-render because obviously window.xx doesn't exist.

If you can just shoot me an example or set me on the right path to getting it on the server side then I can get this going.

On the TransferHttp is there an example on it's usage somewhere in the code? All I saw was linked to the internationalization stuff. We have a client library that is automatically generated from our WebAPI code so I need to adapt what you have to work in that context.

Thanks! Sorry for the move, must be exhausting!

@MarkPieszak
Copy link
Contributor

So really all you want to do it create a Class/service that you add to providers in the main.server file (these are depedencies injected only for the server rendered portion), provide that Class the properties / static properties (whatever you need) here, and during browser renders, have it pluck window.TRANSFER_CACHE when those properties exist there. That way you'll be using it in both 👍

I should probably add some demos of this concept!

@JohnGalt1717
Copy link

Except window.XXX doesn't exist on the server side because it isn't in nodejs. It only works (and it works fine) on the browser side.

I've added the following to my main.server.ts:

const setupOptions: IEngineOptions = {
appSelector: '',
ngModule: ServerAppModule,
request: params,

providers: [
  {
    provide: 'APP_SETTINGS', useValue: <IAppSettings>{
      'serviceUrl': params.data.serviceUrl,
      'clientId': params.data.clientId,
      'url': params.data.url
    }
  }
]

};

and

return ngAspnetCoreEngine(setupOptions).then(response => {
// Apply your transferData to response.globals
response.globals.transferData = createTransferScript({
serviceUrl: params.data.serviceUrl,
url: params.data.url,
clientId: params.data.clientId
});

return {
  html: response.html,
  globals: response.globals
};

});

The transfer script correctly sets window.xxx on the client side but if you try and access it on a server side pre-render it fails saying that window is undefined.

Further if I try and import "APP_SETTINGS in my app.module and pass that in to a function (for static provision of our api client) like this:

import { ORIGIN_URL, APP_SETTINGS, IAppSettings } from './shared/constants/baseurl.constants';

...

providers: [
...
{ provide: APP_INITIALIZER, useFactory: initializeConfig, deps: [CADLearningClient, AppConfig, Injector, Http, APP_SETTINGS], multi: true }
]

and then in initializeConfig I add a param for it, the object is initialized but there is nothing in it.

Alternatively if I use an InjectionToken as a string and change the above provider creation to just a string value the injectiontoken is always null in the app.module.ts.

Hence why I keep banging away at this topic. It just isn't working as you describe. It did work in the javascriptservices Angular 2 version because it forked the Zone and you could add the values as properties on the zone and then access it with Zone.current.get("XXX") but that doesn't work now. I even tried adding it back and as far as I could tell it never inserted them. (and it's very hard to tell because it's virtually impossible to set break points and inspect this stuff which is a major problem that needs to be resolved. Vs code should be setup so that it will debug the .net, the browser and the nodejs execution with a compound debugging session but I can't get the node debugging to actually work properly and set break points so I haven't been successful in this either which is causing major wastes of time in our team because you just get generic (mostly noise) errors in the debug console in VS.net or VS Code without being able to see what's really going on inside of node and the code that's being executed as soon as you have server side pre-rendering enabled.

I'm at 3 days trying to get this to work. The angular 4 documentation is truly awful in this regard and the opacity of JavaScriptServices on top of node makes this an exercise in frustration.

@danobri
Copy link
Contributor

danobri commented Jun 3, 2017

@MarkPieszak @SteveSandersonMS What's the possibility of using this template and targeting the full .NET framework? I tried passing in the -F switch, but looks like the template generator does not support full framework versions. Been working with the template on a project that uses Windows Auth, and just discovered that I need functionality from the System.DirectoryServices library, which unfortunately has not yet been ported to core yet - see CoreFx #2089. If it's possible to target the full framework with this template, would it be practical to convert my existing project? Any information / insight you can provide would be greatly appreciated. Thanks again for all your work on this template!

@danobri
Copy link
Contributor

danobri commented Jun 3, 2017

I just changed the in my csproj to net461, and everything seems to be working fine. So apparently it's not a problem... Is that would you would expect?

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.

None yet