-
Notifications
You must be signed in to change notification settings - Fork 519
Conversation
@MeirionHughes, It will cover your contributions to all .NET Foundation-managed open source projects. |
@MeirionHughes, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request. |
{ test: /\.css$/, loaders: [ 'style-loader', 'css-loader' ] }, | ||
{ test: /\.(png|woff|woff2|eot|ttf|svg)$/, loader: 'url-loader?limit=100000' }, | ||
{ test: /\.json$/, loader: 'json-loader' } | ||
module: { // (4) |
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.
// (4)
? ;)
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.
removed
var path = require('path'); | ||
var webpack = require('webpack'); | ||
var AureliaWebpackPlugin = require('aurelia-webpack-plugin'); | ||
const isDevBuild = process.argv.indexOf('--env.prod') < 0; |
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.
Seems quite ugly. The --env.YYY
option is there so you can module.exports = function(env)
where env
will be an object containing the passed in env.YYY
.
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.
switched to what the others are using:
module.exports = (env) => {
const isDevBuild = !(env && env.prod);
Deferring to @niieani When he says it's good, then I'm happy :) |
{ test: /\.css$/, loaders: [ 'style-loader', 'css-loader' ] }, | ||
{ test: /\.(png|woff|woff2|eot|ttf|svg)$/, loader: 'url-loader?limit=100000' }, | ||
{ test: /\.json$/, loader: 'json-loader' } | ||
module.exports = (env) => { |
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.
You can set a default empty object (env = {}) =>
here and you won't have to env &&
every time it's used :) Also, it might be arguably better to use destructuring here (maybe even with reassignment), like: ({prod} = {})
.
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.
done. :)
|
||
const bundleOutputDir = './wwwroot/dist'; | ||
|
||
return [{ |
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.
Why return an array?
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.
remnant of either the old config or from the one of the other templates. /removed.
@niieani I've checked the production build with: webpack --config webpack.config. vendor.js -p
webpack -p
dotnet run --env Production it appears to be working fine (named slots working too). |
Why not use |
I've been told that includeAll was added to make it easier to migrate older projects; moving forward though, it places some limitations on how your code can be optimised and so you should generally use PLATFORM.moduleName. |
@nmocruz It does have drawbacks, though:
Overall you're missing out quite a lot on what webpack can offer. This is like going back to an AMD + gulp build that is just taking all files in all source and libs and concat them together in a single big file. |
@jods4 Ok, make sense. I need to get used to the idea of use PLATFORM.moduleName('my-module'). |
For libraires it's important to have a common, easily recognizable syntax (because JS is hard to analyze). You should also find that the places where module names appear are usually quite few. |
|
@SteveSanderson This is approved and ready to go on our side. |
@SteveSanderson any estimate on when this merge will occur? |
I think this account is probably the one he uses (its more active) @SteveSandersonMS |
@SteveSandersonMS @EisenbergEffect Any updates? |
@SteveSandersonMS We definitely need this merged ASAP...please. |
@MeirionHughes Great job on this! A really clean, straightforward PR. Also I'm impressed by the improvements to Aurelia itself - the new HMR functionality works brilliantly. I'll now work on publishing the updated template package to NuGet. |
Thanks @SteveSandersonMS ! |
todo: