Add AureliaSpa template #398

kmkatsma · 2016-10-22T15:57:50Z

This PR adds a new AureliaSpa folder to the existing templates folder.

Made attempts to be as consistent as possible with existing templates, but this does follow different webpack template build process - it uses a single webpack config utilizing the AureliaWebpackPlugin. However, I tried to keep the WebPack configuration consistent with the configurations that exist in official Aurelia webpack skeletons, except EasyWebpack has been removed for closer match to the existing webpack templates in this repo.

Other things of note with regard to comparison with other templates:

The nav css had to be modified to deal with the default class added by aurelia to elements (perhaps there is another way).
The menu is build dynamically, instead of statically
Single boot file, given no server side rendering
The fetchdata.ts has some typings overrides added specifically to deal with some current issues in "official" typings files

@SteveSandersonMS, @MarkPieszak, @EisenbergEffect

Added @typings/node to package.json, removed direct typings reference Small webpack.config.js and tsconfig.json changes for similarity to Angular2 version

dnfclas · 2016-10-22T15:57:54Z

Hi @kmkatsma, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

dnfclas · 2016-10-22T16:04:09Z

@kmkatsma, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

SteveSandersonMS · 2016-10-24T12:58:44Z

Thanks @kmkatsma! Overall this looks great. You're identified most of the limitations yourself above, and I appreciate that they are beyond your control. Thanks for making it consistent with the other templates where possible.

I'll now review and comment on the actual code. Since I don't know Aurelia, I'll wait for @EisenbergEffect to comment on whether Aurelia is being used optimally. Mostly I'll be limited to commenting on the cosmetic aspects of the code - hope you don't mind my rather trivial comments!

EisenbergEffect · 2016-10-24T13:13:29Z

templates/AureliaSpa/ClientApp/app/components/navmenu/navmenu.html

+ <div class='clearfix'></div>
+ <div class='navbar-collapse collapse'>
+ <ul class='nav navbar-nav'>
+ <li class="${row.isActive ? 'link-active' : ''}" repeat.for = "row of router.navigation" >


For clarity, I would put the repeat.for first on the li, before the class binding.

EisenbergEffect · 2016-10-24T13:18:37Z

I left one minor style comment related to the Aurelia code. Everything else looks fine to me :)

MarkPieszak · 2016-10-24T13:26:14Z

Forgot to say, great job @kmkatsma ! 💯 👍

EisenbergEffect · 2016-10-24T13:29:25Z

Agree with @MarkPieszak This is a really awesome contribution. When it this goes live, if you would like to @kmkatsma we can feature you on our official blog for this work. I'm sure a lot of people will be interested.

SteveSandersonMS

@kmkatsma Thanks again for this excellent contribution! I hope you don't mind these comments - most of them are pretty trivial.

SteveSandersonMS · 2016-10-24T12:59:05Z

templates/AureliaSpa/Aurelia.xproj

+ <Import Project="$(VSToolsPath)\DotNet\Microsoft.DotNet.Props" Condition="'$(VSToolsPath)' != ''" />
+ <PropertyGroup Label="Globals">
+ <ProjectGuid>33d8dad9-74f9-471d-8bad-55f828faa736</ProjectGuid>
+ <RootNamespace>Angular2</RootNamespace>


Please update this

SteveSandersonMS · 2016-10-24T13:01:32Z

templates/AureliaSpa/ClientApp/app/components/app/app.html

@@ -0,0 +1,14 @@
+<template>


Inconsistent quote style in this file. In TypeScript let's use single-quotes. In HTML templates, I don't know if Aurelia community has a standard, but can you find out what it is and use that?

We prefer single quotes in JS/TS and double quotes in HTML.

SteveSandersonMS · 2016-10-24T13:02:27Z

templates/AureliaSpa/ClientApp/app/components/app/app.html

+ </div>
+ </div>
+ </div>
+</template>


Missing trailing linebreak which is present in most files. Could you add one here and to any other TS/JS/HTML/CSS file where it's missing?

SteveSandersonMS · 2016-10-24T13:04:48Z

templates/AureliaSpa/ClientApp/app/components/app/app.ts

@@ -0,0 +1,19 @@
+import {Aurelia} from 'aurelia-framework';


Some few formatting issues in this file:

missing spaces around { ImportedModule }

The config.map object looks pretty strange. Not sure if you were trying to line things up with tabs or something, but it's not lined up now. Also some of the key-value pairs have spaces after the colon and others don't. Could you lay this code out more conventionally?

SteveSandersonMS · 2016-10-24T13:05:12Z

templates/AureliaSpa/ClientApp/app/components/app/app.ts

+ }
+}
+
+


Line 18 seems like unnecessary whitespace, then there's a missing trailing linebreak.

SteveSandersonMS · 2016-10-24T13:24:13Z

templates/AureliaSpa/webpack.config.js

+ entry: {
+ 'app': [], // <-- this array will be filled by the aurelia-webpack-plugin
+ 'aurelia-bootstrap': aureliaBootstrap,
+ 'aurelia-modules': aureliaModules.filter(pkg => aureliaBootstrap.indexOf(pkg) === -1)


Why are these dependencies going into a separate bundle? Couldn't they just be included in aurelia-bootstrap? (which they would be by default if this line wasn't here)

SteveSandersonMS · 2016-10-24T13:24:46Z

templates/AureliaSpa/wwwroot/web.config

@@ -0,0 +1,9 @@
+<?xml version="1.0" encoding="utf-8"?>


I don't think this file is needed. It's not present in any of the other templates.

SteveSandersonMS · 2016-10-24T13:26:41Z

templates/AureliaSpa/webpack.config.js

+ },
+ plugins: [
+ new webpack.ProvidePlugin({
+ regeneratorRuntime: 'regenerator-runtime', // to support await/async syntax


Is that needed? I didn't see any await/async anywhere. Can this be simplified?

SteveSandersonMS · 2016-10-24T13:26:54Z

templates/AureliaSpa/webpack.config.js

+ Promise: 'bluebird', // because Edge browser has slow native Promise object
+ $: 'jquery', // because 'bootstrap' by Twitter depends on this
+ jQuery: 'jquery', // just an alias
+ 'window.jQuery': 'jquery' // this doesn't expose jQuery property for window, but exposes it to every module


Do we need all three of these aliases for jQuery?

SteveSandersonMS · 2016-10-24T13:29:59Z

templates/AureliaSpa/Startup.cs

+ {
+ app.UseDeveloperExceptionPage();
+ app.UseWebpackDevMiddleware(new WebpackDevMiddlewareOptions {
+ HotModuleReplacement = false


I'm not sure we'll get away with just setting this to false. The problem is, people will change it to true, and then you get a really weird and non-descriptive error that people will regard as a bug. It's reasonable for people to think that if some option exists that you should be able to use it.

Obviously the ideal solution would be if HMR could be made to work. I'd hope that's not impossible, since it's quite trivial to enable with Angular/Knockout/React, but I don't know if Aurelia does something that can't handle it.

If it's not possible to make work, can you somehow stop the unrelated-seeming error message, and replace it with something like "Sorry, Hot Module Replacement is not supported with Aurelia"?

kmkatsma · 2016-10-24T17:45:20Z

@SteveSandersonMS - thanks - a lot of good comments here, responded to couple, but too many to address individually. =) I will look at them all. I was trying to clean things up for consistency, and this helps a ton.

@MarkPieszak @EisenbergEffect - thanks, appreciate it! I would definitely be good with a feature on the Aurelia blog! These kinds of projects are really important - anything we can do to help developers get a foothold into technologies like these is worthwhile. And I think Aurelia deserved a place here as well. =)

EisenbergEffect · 2016-10-25T18:42:30Z

I think doing this sort of thing in a constructor is bad, yes. I wouldn't
recommend that in any language. Constructor code should always by
synchronous.

On Mon, Oct 24, 2016 at 6:54 AM, Steve Sanderson notifications@github.com
wrote:

@SteveSandersonMS commented on this pull request.

In templates/AureliaSpa/ClientApp/app/components/fetchdata/fetchdata.ts
#398:

@@ -0,0 +1,27 @@
+///
+///
+import {HttpClient} from 'aurelia-fetch-client';
+import {inject} from 'aurelia-framework';
+
+@Inject(HttpClient)
+export class Fetchdata {

public forecasts: WeatherForecast[];

http: HttpClient;

@EisenbergEffect https://github.com/EisenbergEffect It's the
simplicity/extensibility tradeoff. In the Angular/Knockout/React templates
we've followed this exact approach, as we're trying to make the templates
accessible to newcompers and limit the concept count. People who get into
using one of those frameworks won't struggle to create a service layer. In
the ReactRedux template, we do the whole Redux thing which is a whole extra
level of sophistication, but that's just fundamental to doing Redux. I
think for this Aurelia template it's a reasonable choice of simplicity vs
extensibility already, but we could change it if you think it's just flat
out bad.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#398, or mute the
thread
https://github.com/notifications/unsubscribe-auth/AAIBnccSp7t35BhGwypRsjQjHbEvaTTAks5q3LiwgaJpZM4Kd5Pj
.

Rob Eisenberg,
President - Blue Spire
aurelia.io

SteveSandersonMS · 2016-10-25T22:48:07Z

@EisenbergEffect It's a reasonable point but perhaps we're getting a bit side-tracked.

@kmkatsma's Thanks - awaiting your updates :)

kmkatsma · 2016-10-26T02:02:52Z

Have pushed in update in updates for all the review items. There's two things to note regarding resolutions (or somewhat lack thereof ).

For fetchdata, I couldn't eliminate need for the d.ts custom entries using the new types approach. I could move to a typings folder, but I think that is not any better. I also left the api call in constructor for now since it's same as Angular2 template, but would be willing to refactor this.
For dotnet publish, it builds, and I have its output definition matching angular2 template, but I am having issues getting the publish of either Angular2 or Aurelia running in IIS on my dev VM (built off the new windows 10 pro dev images). I spent a fair amount of time on this, working through some stackoverflow answers to no avail. I can spend some more time if we want to get a running confirmation before merging.
HMR - I was able to get app to run with this setting enabled using a hack to webpack config, but it caused issues when running in asp env production mode. So at this point, I put a comment in Startup.cs to the effect that HMR has issues, and to leave the setting false. Definitely this is something that is high priority for developer experience, but the webpack integration with Aurelia is still work in progress. I think the convention based approach (which is advantage from code perspective) actually makes it hard for WebPack to find all its pieces, and so are dependent on the custom plugin to pull in all the pieces and run it.

Let me know thoughts on these things - have learned a ton here going through this! I think the back end integration of these templates is a huge win, and removes a lot of friction we've seen since everything moved to client in a SPA. I can definitely see people wanting to get more things into them. =)

kmkatsma · 2016-10-28T00:46:27Z

Added a commit that fixed issues with dotnet publish.

First, when publishing to virtual directory that is not at root, the router was having issues due to base url set to "" in the layout.cshtml. Apparently this works for angular2 in publish mode, but causes issues for aurelia routing. I removed this, and now works in dev and publish mode under IIS virtual directory.

Second change was to remove node_modules from the items included in publish process. It made the publish really big and run really slow, and is not needed since webpack runs as pre-publish step.

There's a funky bug in publish process where the generated web.config keeps appending extra arguments on each run if you don't delete the web.config first. This was causing my publish problems.

niieani

Some comments regarding the Webpack config.

niieani · 2016-10-28T11:46:22Z

templates/AureliaSpa/package.json

+ "jquery": "^2.2.1"
+ },
+ "devDependencies": {
+ "@types/node": "^6.0.45",


What are the node types used for?

niieani · 2016-10-28T11:47:41Z

templates/AureliaSpa/webpack.config.js

+ resolve: { extensions: [ '.js', '.ts' ] },
+ devtool: isDevBuild ? 'inline-source-map' : null,
+ entry: {
+ 'app': [], // <-- this array will be filled by the aurelia-webpack-plugin


Latest Webpack@beta currently errors when an array is empty (it's an ongoing bug). I might be better to instead point it to where the main.js file is, as that should be in the context anyway.

This is changed in the current version too, so I think we're OK.

niieani · 2016-10-28T11:49:27Z

templates/AureliaSpa/webpack.config.js

+var outDir = path.resolve('./wwwroot/dist');
+var baseUrl = '/';
+var project = require('./package.json');
+var aureliaModules = Object.keys(project.dependencies).filter(dep => dep.startsWith('aurelia-'));


This only gets the list of first-level dependencies of Aurelia, which means not all of Aurelia's dependencies might get into the aurelia-modules bundle specified in the entry config.

Good point. However this approach is no longer used in the current version anyway - dependencies are now listed explicitly.

SteveSandersonMS · 2016-10-28T14:59:46Z

Thanks again @kmkatsma - this is now merged :) Also I've published generator-aspnetcore-spa version 0.4.0 in which you'll find the new "Aurelia" option.

I made a few further tweaks - hope you don't mind:

Switched to use the same 'vendor' bundle pattern as the other templates
Switched to use external source maps configured for VS/VSCode compatibility (again, to match the other templates)
Changed the "log or not" choice to depend on which webpack build configuration you used, rather than based on whether you have localhost in your hostname
Minor stylistic tweaks

MarkPieszak · 2016-10-28T15:04:19Z

@niieani The @types/Node is mainly to make Visual Studio happy when it comes to require and other similar things.

Also great! Can't wait to take it for a test drive 💯

EisenbergEffect · 2016-10-28T15:12:38Z

Awesome! @kmkatsma We'd love to have you featured on the official Aurelia blog talking about this new options for our customers :) Please email me and let's make some plans. Thanks for the great work!

ClaudioNunes · 2016-10-28T20:00:48Z

The only thing missing is HMR. Maybe @niieani could help with this.

niieani · 2016-10-28T20:05:13Z

HMR support is planned. Once major Aurelia+Webpack fixes land in November, I might tackle basic HMR in December. We'll see how difficult it will be.

kmkatsma · 2016-10-29T00:47:19Z

@SteveSandersonMS - your changes wrapped this up nicely, I think!

ClaudioNunes · 2016-11-15T17:24:55Z

@niieani any news about HMR support?

niieani · 2016-11-15T17:33:05Z

@ClaudioNunes not yet. But work is on-going. See aurelia/skeleton-navigation#714 for a couple of updates (HMR for CSS works).

acu-kkatsma and others added 6 commits October 18, 2016 12:06

updates for aurelia

600aa17

clean up

f24fce8

Add typings for node

28c778e

removed local gitignroe

1563578

Added in missing _placeholder.txt

12120ef

Added @typings/node to package.json, removed direct typings reference Small webpack.config.js and tsconfig.json changes for similarity to Angular2 version

Changes to C# Namespace for uniformity to other templates

b363ed7

dnfclas added the cla-required label Oct 22, 2016

dnfclas added cla-signed and removed cla-required labels Oct 22, 2016

Cleanup - 2.0 typings format, script tags to layout.cshtml

fe5059c

EisenbergEffect reviewed Oct 24, 2016

View reviewed changes

SteveSandersonMS suggested changes Oct 24, 2016

View reviewed changes

kmkatsma added 4 commits October 25, 2016 18:40

First round of code review cleanup

790b719

Booting aurelia into a div in Index.cshtml

e8b2ba8

HMR workaround applied (seems best "dev experience" for now)

5747fb8

Removed commented line, reverted HMR hack

48a8162

fixed issues with publish

68bf670

niieani reviewed Oct 28, 2016

View reviewed changes

SteveSandersonMS approved these changes Oct 28, 2016

View reviewed changes

SteveSandersonMS pushed a commit that referenced this pull request Oct 28, 2016

Add AureliaSpa template (#398)

e60ea04

SteveSandersonMS closed this Oct 28, 2016

mttmccb mentioned this pull request Oct 28, 2016

Support Aurelia #347

Closed

Add AureliaSpa template #398

Add AureliaSpa template #398

Conversation

kmkatsma commented Oct 22, 2016

dnfclas commented Oct 22, 2016

dnfclas commented Oct 22, 2016

SteveSandersonMS commented Oct 24, 2016

Choose a reason for hiding this comment

EisenbergEffect commented Oct 24, 2016

MarkPieszak commented Oct 24, 2016

EisenbergEffect commented Oct 24, 2016

SteveSandersonMS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EisenbergEffect Oct 24, 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

SteveSandersonMS Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmkatsma commented Oct 24, 2016

EisenbergEffect commented Oct 25, 2016

@SteveSandersonMS commented on this pull request.

SteveSandersonMS commented Oct 25, 2016

kmkatsma commented Oct 26, 2016

kmkatsma commented Oct 28, 2016

niieani left a 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

SteveSandersonMS commented Oct 28, 2016

MarkPieszak commented Oct 28, 2016 • edited Loading

EisenbergEffect commented Oct 28, 2016

ClaudioNunes commented Oct 28, 2016

niieani commented Oct 28, 2016

kmkatsma commented Oct 29, 2016

ClaudioNunes commented Nov 15, 2016

niieani commented Nov 15, 2016

EisenbergEffect Oct 24, 2016 •

edited

Loading

SteveSandersonMS Oct 24, 2016 •

edited

Loading

MarkPieszak commented Oct 28, 2016 •

edited

Loading