aspnet / JavaScriptServices

[Archived] This repository has been archived
Apache License 2.0
3.04k stars 519 forks source link

Angular SPA SSR Template bug with Angular 6 #1619

Closed speige closed 6 years ago

speige commented 6 years ago

I converted this template https://docs.microsoft.com/en-us/aspnet/core/spa/angular?view=aspnetcore-2.1&tabs=visual-studio to Angular 6. Everything worked fine initially, but then an update to @angular-devkit/build-angular npm package (from version 0.5.5 to 0.5.6) broke it.

The reason I'm not submitting this issue to the Angular repo is I think it's actually a bug in https://github.com/aspnet/JavaScriptServices/blob/dev/src/Microsoft.AspNetCore.SpaServices.Extensions/Util/EventedStreamReader.cs that wasn't noticed until now. Basically the AngularCliBuilder.cs waits for "ng build:ssr" to finish by watching the console output & looking for "Date" in the output. I haven't done a "diff" of the actual ASCII values in the console output between the two angular versions, but visually they look almost identical, except the new version has an extra newline, otherwise they look the same.

This is the error I'm getting: image

Here's the visual difference between the console outputs: working: image

broken: image

I need to research more, but I believe the issue is with this function in EventedStreamReader.cs:

        private async Task Run()
        {
            var buf = new char[8 * 1024];
            while (true)
            {
                var chunkLength = await _streamReader.ReadAsync(buf, 0, buf.Length);
                if (chunkLength == 0)
                {
                    OnClosed();
                    break;
                }

                OnChunk(new ArraySegment<char>(buf, 0, chunkLength));

                var lineBreakPos = Array.IndexOf(buf, '\n', 0, chunkLength);
                if (lineBreakPos < 0)
                {
                    _linesBuffer.Append(buf, 0, chunkLength);
                }
                else
                {
                    _linesBuffer.Append(buf, 0, lineBreakPos + 1);
                    OnCompleteLine(_linesBuffer.ToString());
                    _linesBuffer.Clear();
                    _linesBuffer.Append(buf, lineBreakPos + 1, chunkLength - (lineBreakPos + 1));
                }
            }
        }

I think it should call OnCompleteLine(_linesBuffer.ToString()); a final time at the end of the function if there's anything still in the buffer.

I've created a repo for reproducing this at https://github.com/speige/Angular6DotNetCoreSpaTemplateBug The "master" branch is the original angular 5 template. The "Angular6Working" branch is an straight upgrade to v6 with the @angular-devkit/build-angular npm package on 0.5.5. The "Angular6broken" branch is identical except with @angular-devkit/build-angular on 0.5.6

The "Angular6Working" branch might be useful in order to upgrade the official template to Angular 6 (still in RC, but should be oficially released any day now).

On a side note, for Angular6 upgrade, a change should be done in https://github.com/aspnet/JavaScriptServices/blob/dev/src/Microsoft.AspNetCore.SpaServices.Extensions/AngularCli/AngularCliBuilder.cs

            var npmScriptRunner = new NpmScriptRunner(
                sourcePath,
                _npmScriptName,
                "--watch",
                null);

The "--watch" is no longer a valid command line argument in Angular 6 CLI. I believe if you do "ng serve" it automatically does --watch, if you do "ng build" it doesn't.

I worked around this issue by changing the "build:ssr" command in my package.json from

    "build:ssr": "ng build --configuration=production --project=ssr"

to

    "build:ssr_ignore_dash_dash_watch_from_AngularCliBuilderDotCS": "ng build --configuration=production --project=ssr ",
    "build:ssr": "npm run build:ssr_ignore_dash_dash_watch_from_AngularCliBuilderDotCS"

which basically causes the extra command line argument to be discarded before "ng build" is called. Otherwise Angular throws an error about an invalid command line argument.

I'll test my theory with EventedStreamReader.cs & assuming it fixes this issue I'll submit a pull request.

speige commented 6 years ago

I tested my theory & it resolved the issue in my repo. I've created a pull request. https://github.com/aspnet/JavaScriptServices/pull/1620 This should be backwards compatible with past versions of Angular.

The pull request doesn't address the --watch issue because I found a workaround for my app & I don't want to cause issues for users of old versions of Angular.

@SteveSanderson The pull request doesn't upgrade the built-in template to Angular6. You could do a diff between https://github.com/speige/Angular6DotNetCoreSpaTemplateBug (Angular6Working branch) to see how to update the current template, if desired. Once this pull request is accepted you could also update the @angular-devkit/build-angular npm package to latest (0.5.7)

asadsahi commented 6 years ago

@speige I tried to follow your instructions but unable to get the project building. have you tried with final angular 6?

speige commented 6 years ago

@asadsahi

Yes, my workaround is successful for me on angular v6 final.

I'm curious how you implemented my workaround? The pull request hasn't been accepted, so you'd have to download the source to aspnet/JavaScriptServices, make the change, compile it, & replace the official nuget package/dlls with your modified dlls in your angular project. Did you do all that?

You would also need to modify your package.json to workaound the --watch issue.

Have you made both of these changes?

What errors are you seeing?

naveedahmed1 commented 6 years ago

@SteveSandersonMS any update on this?

naveedahmed1 commented 6 years ago

Ok I downloaded/cloned the repo https://github.com/speige/Angular6DotNetCoreSpaTemplateBug/tree/angular6Working and after restoring npm packages tried to run the project, it didn't work.

Then I tried running below commands through command prompt in ClientApp and everything worked perfectly.

npm install -g @angular/cli

npm install @angular/cli

ng update @angular/cli

ng update @angular/core
SteveSandersonMS commented 6 years ago

Angular 6 support is being worked on at https://github.com/aspnet/templating/pull/515. This isn't in the 2.1 release but hopefully should be included in whatever version ships after (maybe it will be called 2.2, though I don't know if the version number has been decided yet).

speige commented 6 years ago

@SteveSandersonMS, did you look at my pull request before closing this?

I understand that your normal upgrade process is being followed in https://github.com/aspnet/templating/pull/515 which is to take the latest angular repo & work backwards to make the .net version. However, I believe there's a bug in https://github.com/aspnet/JavaScriptServices/blob/dev/src/Microsoft.AspNetCore.SpaServices.Extensions/Util/EventedStreamReader.cs which needs to be fixed before SSR for angular 6 will be successful.

SteveSandersonMS commented 6 years ago

Yes, your PR is still active. I expect you are correct that this needs to be fixed. Thanks for submitting it!

naveedahmed1 commented 6 years ago

It would be really great if we could have a fix anytime soon.

bastienlemaitre commented 6 years ago

Any new ?

MakoPhil commented 6 years ago

Good find, @speige. I'm keen to see a fix for this soon as well. Should we be following this issue or one of the other ones for updates?

speige commented 6 years ago

The fix for this particular bug is in pull request #1620 I believe they closed this issue as a duplicate of other Angular6+VisualStudioSPATemplate issues.

I've switched to my own modified version of the aspnet spa template because I couldn't wait for official support. I'm not sure the current state of the latest aspnet template or if it still has bugs or not. You'll have to research that on your own. Good luck :)

MakoPhil commented 6 years ago

Thanks, I subscribed to the pull request. If I'm not mistaken, this only affects development, not production. If you are using one of the templates, you should be able to disable prerendering in dev like this:

options.BootModuleBuilder = null;
// Don't use the AngularCliBuilder
// options.BootModuleBuilder = env.IsDevelopment()
//     ? new AngularCliBuilder(npmScript: "build:ssr")
//     : null;
speige commented 6 years ago

That's probably true, I haven't tried it. I just compiled Microsoft.AspNetCore.SpaServices.Extensions.dll myself off my forked code & I'm using that dll instead of the one from NuGet. I'll switch back to the official version once the PR is accepted.