Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(javascriptservices): update for .net core 2.0 #741

Merged
merged 3 commits into from
Oct 12, 2017

Conversation

grufffta
Copy link
Contributor

@grufffta grufffta commented Sep 2, 2017

Hi,

I have updated the template for the javascriptservices to fix issues #734 and #701
I have updated the references to use the new meta package, fixed an issue where the index cshtml incorrectly had boot as the Aurelia app and allowed .net core to find the production files produced in the dist folder.

I did notice there was a failing test (nesting of dist folders) but this was before I made any changes so I didn't want to touch that with this pull request.

Hope you don't mind.
Thanks
Gareth

@CLAassistant
Copy link

CLAassistant commented Sep 2, 2017

CLA assistant check
All committers have signed the CLA.

@grufffta
Copy link
Contributor Author

grufffta commented Sep 2, 2017

Some notes:

  • au run - runs the built in webpack server on port 8080 using the index.html file
  • dotnet run runs the asp.net core build and serves index.cshtml on port 5000, if au build hasn't been run then the files will not exist in the dist folder.

Looking at the Aurelia template on the JavaScriptServices repository they have an additional step to check and run webpack. In the current cli it is just has a step for publish but this also has an unnecessary call to webapack.vendor.config that doesn't exist.

Happy to fix these issues as well if needed?

@JeroenVinke
Copy link
Collaborator

Awesome, thanks a lot! This is a great contribution. Yes, please make any improvements you would like to make. The one failing test is correct, you can ignore it.

I see that you've removed the Configuration setup, does that mean that appsettings.json no longer will work?

When you're done, please make sure that the commits use the message format. Examples can be found here.

@JeroenVinke
Copy link
Collaborator

au run - runs the built in webpack server on port 8080 using the index.html file
dotnet run runs the asp.net core build and serves index.cshtml on port 5000, if au build hasn't been run then the files will not exist in the dist folder.

Would it still be able to detect changes? Is there any setup possible that wouldn't require us to do au build?

@grufffta
Copy link
Contributor Author

grufffta commented Sep 3, 2017

I see that you've removed the Configuration setup, does that mean that appsettings.json no longer will work?

The configuration changed in .net core 2 and is moved to Program.BuildWebHost the extension method on WebHost CreateDefaultBuilder configures .net core 2 in the same way with as the old .net core 1 default config.

Would it still be able to detect changes? Is there any setup possible that wouldn't require us to do au build?

Yes, It should just be automatic so when you run dotnet run the build is produced and monitored for changes. au would still do this in the background as a pre-build step triggered by dotnet.

@grufffta
Copy link
Contributor Author

grufffta commented Sep 4, 2017

I almost had this working yesterday but seemed to have hit the dreaded webpack rebuild loop.
This only seems to happen when the files already exist in the dist folder, adding the clean web pack plugin should resolve this. The HTML web page plugin overrides the index.cshtml file and is served first, so that needs to be removed as well. I can make a custom webpack.netcore.config.js file that can modify the configuration to make it work with the .net core middleware without affecting the main config generated by the cli.

@JeroenVinke
Copy link
Collaborator

I wonder what is different in our setup as opposed to JavascriptServices that we need to do a build initially. I'd like to stay as close to the javascriptservices setup as possible. If you could look into this then that would be a big help

The webpack build was broken with .net core 2.0
The SpaServices middleware is incompatible with the cli webpack config
This adds a config file for webpack that fixes it and was added to the
new project build.

Realtes to issues aurelia#734 and aurelia#701, clean-webpack-plugin needs to be
installed otherwise webpack gets stuck in a rebuilding loop
see webpack/watchpack#25 and referenced issues
@grufffta
Copy link
Contributor Author

grufffta commented Sep 4, 2017

It was my mistake and it was only an initial build for production build that triggered webpack from the csproj.
The cli instructs the user to use au build for production and the template calls this for publish as well but not if the user starts it in Production mode (production is default for dotnet run if the ASPNETCORE_ENVIRONMENT variable is not set to Development.

@grufffta grufffta changed the title Update for asp.net core 2.0.0 fix(javascriptservices): update for .net core 2.0 Sep 5, 2017
@jblackburn21
Copy link

Any updates on getting this merged in? I pulled in the changes, and they look good so far.

@JeroenVinke
Copy link
Collaborator

I aim to take a look at PR's in the next week or so

@JeroenVinke JeroenVinke merged commit 5a43285 into aurelia:master Oct 12, 2017
@JeroenVinke
Copy link
Collaborator

Great work, thank you @grufffta. One question though, I got the following error when I tried dotnet run:

C:\Development\Aurelia\Apps\aurelia-cli-aspnetcore2\webpack.netcore.config.js:7
  let config = { ...originalConfig };
                 ^^^
SyntaxError: Unexpected token ...
    at createScript (vm.js:53:10)
    at Object.runInThisContext (vm.js:95:10)
    at Module._compile (module.js:543:28)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Module.require (module.js:498:17)
    at require (internal/module.js:20:19)
    at Object.requireNewCopy (C:\Development\Aurelia\Apps\aurelia-cli-aspnetcore2\node_modules\aspnet-webpack\RequireNewCopy.js:16:16)) ---> Microsoft.AspNetCore.NodeServices.HostingModels.NodeInvocationException: Unexpected token ...
C:\Development\Aurelia\Apps\aurelia-cli-aspnetcore2\webpack.netcore.config.js:7
  let config = { ...originalConfig };
                 ^^^

Replacing that line with let config = originalConfig; fixed it though. Any thoughts?

@mttmccb
Copy link

mttmccb commented Oct 12, 2017

Which version on node are you using? I remember reading the spread operator was a recent addition.

@JeroenVinke
Copy link
Collaborator

I am on v7.10.0. I wonder if it's really necessary to clone the config

@grufffta
Copy link
Contributor Author

I can't see any issues with using let config = originalConfig; although it is strange it didn't complain about the spread operator being used to recreate the plugin array.

@ItWorksOnMyMachine
Copy link

This is really great! I'm still running in to a problem with locating static resources that webpack copies into the dist folder. Specifically font files used in css. Webpack copies them to the dist folder but when they are requested they are requested at the root and hence a 404. Any thoughts?

@JeroenVinke
Copy link
Collaborator

@grufffta any thoughts?

@grufffta
Copy link
Contributor Author

grufffta commented Nov 9, 2017

I presume this is font awesome, I managed to reproduce the issue and get the urls corrected by adding the following rule above the existing font file-loader.

{
        test: /fontawesome-webfont\.(eot|ttf|svg|woff|woff2)(\?v=[0-9]\.[0-9]\.[0-9])?$/i, loader: 'file-loader',
        options: {
          name: 'fonts/[name].[ext]',
          publicPath: '/dist/'
        }
      },

The FA css references the fonts with a ../fonts/ ref. I haven't got much more time to look at this tonight and I see the fonts now load in the devtools but on my machine the icons were just rendering a square still

@grufffta
Copy link
Contributor Author

grufffta commented Nov 9, 2017

So I have looked at this a bit more and resolved the rendering issue.

@ItWorksOnMyMachine
The options for all the font loaders need to have publicPath: '/dist/' and I'd imagine this would need to be done for the image loaders as well.

@JeroenVinke
We could make a change to the webpack.netcore.config.js to set the publicPath on the options of the url-loader and file-loader rules. What do you think ?

grufffta added a commit to grufffta/cli that referenced this pull request Nov 9, 2017
…static resources

The webpack build moves files into the /dist/ folder and resources are loaded from the root '/' publicPath
This adds a change to the webpack.netcore.config.js file that customizes webpack to work with javascript services by adding the publicPath to the url and file loaders.

Relates to a discussion on a previous PR aurelia#741
@ItWorksOnMyMachine
Copy link

Sorry, I posted this question to a few places and completely forgot I posted it here as well. I did resolve it, and it was as "simple" as setting the publicPath property in my webpack.config.js file to publicPath: baseUrl + 'dist/'. In this case, baseUrl == '/'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants