search

aurelia / hot-module-reload

Core functionality for Aurelia's hot-module-reolad (HMR) capabilities, which is shared by all loaders and tools.
MIT License
25 stars 8 forks source link

SCSS hot reload is not working #16

Closed AdamWillden closed 6 years ago

AdamWillden commented 7 years ago

@EisenbergEffect thanks for doing the release, unfortunately the new feat in #12 doesn't seem to work for me. @niieani, @jods4 did you try it out at the time?

I'm submitting a bug report

Please tell us about your environment:

Current behavior: All my dependencies are up to date

{
  "version": "1.0.0",
  "sasslintConfig": "node_modules/siopa-package-tools/.sass-lint.yml",
  "private": true,
  "scripts": {
    "test": "jest",
    "start": "webpack"
  },
  "dependencies": {
    "aurelia-bootstrapper": "^2.1.1",
    "aurelia-dependency-injection": "^1.3.2",
    "aurelia-framework": "^1.1.5",
    "aurelia-pal": "^1.4.0",
    "aurelia-router": "^1.4.0"
  },
  "devDependencies": {
    "@types/extract-text-webpack-plugin": "^3.0.0",
    "@types/jest": "^21.1.2",
    "@types/node": "^8.0.32",
    "aspnet-webpack": "^2.0.1",
    "aurelia-hot-module-reload": "^0.2.0",
    "aurelia-polyfills": "^1.2.2",
    "aurelia-webpack-plugin": "^2.0.0-rc.5",
    "css-loader": "^0.28.7",
    "extract-text-webpack-plugin": "^3.0.0",
    "file-loader": "^1.1.5",
    "gulp": "github:gulpjs/gulp#4.0",
    "import-glob-loader": "^1.1.0",
    "node-sass": "^4.5.3",
    "sass-loader": "^6.0.6",
    "siopa-package-tools": "^1.0.9",
    "style-loader": "^0.19.0",
    "ts-loader": "^2.3.7",
    "ts-node": "^3.3.0",
    "tslib": "^1.7.1",
    "tslint": "^5.5.0",
    "typescript": "^2.5.2",
    "webpack": "^3.6.0",
    "webpack-hot-middleware": "^2.19.1"
  }
}

Given the following component, when changing the color, an error is displayed in console.

// relevant webpack config line
{ test: /\.scss$/i, issuer: /\.html$/i, loader: 'css-loader!sass-loader' },
<template class="c-header-bar">
  <require from="./header-bar.scss"></require>
  My Header Text
</template>
.c-header-bar {
  color: blue;
}
process-update.js:115 [HMR] Cannot check for update (Full reload needed)
handleError @ process-update.js:115
process-update.js:116 [HMR] TypeError: Cannot read property 'resources' of undefined
    at HmrContext.29.HmrContext.reloadCss (http://localhost:5000/dist/app.js?v=QNzl5O3zgddSQAkYZUauFSbxSVlDVeKMHEZPOYHMqfk:10355:29)
...ETC
handleError @ process-update.js:116
aurelia-hot-module-reload.js:235 Uncaught (in promise) TypeError: Cannot read property 'resources' of undefined
    at HmrContext.29.HmrContext.reloadCss (aurelia-hot-module-reload.js:235)
    at Object.hot (aurelia-hot-module-reload.js:79)
    at aurelia-loader-webpack.js:167
    at Object.hotApply [as apply] (bootstrap 8fa308dbf2e403c4d37c:580)
    at cb (process-update.js:52)
    at process-update.js:68
    at <anonymous>

Expected/desired behavior:

Given the following component, when changing the color, then the rendered text should change color without console errors.

If I use .css files instead with the same setup and following webpack config line everything works

        { test: /\.css$/i, issuer: /\.html$/i, loader: 'css-loader' },
jods4 commented 6 years ago

I did the change in #12 mostly because it's the same code as another PR I did in aurelia-templating-resources, to keep things consistent. I think I had a basic LESS example working, but to be honest that was long ago and I'm not 100% sure. There has been major Webpack releases since then so even if it worked it might have been broken in the meantime.

I'm confident this could work with HMR but someone needs to debug the error and fix it.

AdamWillden commented 6 years ago

Found some time to look into this. The culprit is obvious. It's fixed as a css-resource-plugin but the resource is under scss-resource-plugin.

image

Any thoughts on the best way to determine the plugin to look for? I assume the same code exists elsewhere.

jods4 commented 6 years ago

To clarify:

  1. do you transpile your scss into css with a Webpack loader? So at runtime, css-resource-plugin is right.
  2. or do you pass scss code to the runtime, hoping it will use a converter?

I'm thinking 1. is the right way to do it with Webpack and should work?

AdamWillden commented 6 years ago

See the details in the issue above. I think I'm doing 2??

FWIW I just changed the distributed code as follows and it works (just as a test).

From:

        var cssPluginModuleId = this.loader.applyPluginToUrl(moduleId, 'css-resource-plugin');
        console.log("Handling HMR for " + moduleId);
        delete this.loader.moduleRegistry[moduleId];
        delete this.loader.moduleRegistry[cssPluginModuleId];
        var analyzedModule = this.moduleAnalyzerCache["css-resource-plugin!" + moduleId];

To:

        var cssPluginModuleId = this.loader.applyPluginToUrl(moduleId, 'scss-resource-plugin');
        console.log("Handling HMR for " + moduleId);
        delete this.loader.moduleRegistry[moduleId];
        delete this.loader.moduleRegistry[cssPluginModuleId];
        var analyzedModule = this.moduleAnalyzerCache[cssPluginModuleId];
AdamWillden commented 6 years ago

Picture vs thousand words: image

AdamWillden commented 6 years ago

The plugin name is convention-based upon the file extension name so perhaps we can ignorantly build the plugin name within hot-module-reload

...but part of me says there should be an interface we can use in the place where that convention has been implemented. i.e. a viewEngine.getResourcePluginName should probably exist

jods4 commented 6 years ago

Having a static build step with webpack, I don't quite see why you would compile your resources at runtime. Is there a benefit of doing so?

I'm using LESS and I compile everything down to minified CSS before pushing to browser. In my head this is the main/only(?) scenario.

We could try to support both use cases, but that would require more thinking I guess.

AdamWillden commented 6 years ago

I guess I misunderstood (and still do misunderstand) what #12 was for if not this. I'm new to webpack and this is part of my migration from JSPM..

I understood the resulting bundle to contain css not scss. Thus not compiling scss at runtime as you seem to suggest. Is the reason for referencing the scss not just to trigger the compilation into css on change and serve that? Thats what 'css-loader!sass-loader' is doing right?

This is taken from my resulting app.js bundle. Which shows css and not the variable/mixin containing scss that I began with (albeit with \n new lines):

// module
exports.push([module.i, ".c-header-bar {\n  align-items: center;\n  display: flex;\n  flex-wrap: wrap;\n  color: #FF5113;\n  -webkit-border-radius: 10px;\n  -moz-border-radius: 10px;\n  border-radius: 10px; }\n  .c-header-bar__logo {\n    flex: 0 0 auto;\n    margin: 1rem; }\n  .c-header-bar__heading {\n    flex: 0 1 auto;\n    margin: 0 1rem; }\n  .c-header-bar__loader {\n    flex: 1 1 auto;\n    font-size: 2em;\n    margin: 0 1rem; }\n  .c-header-bar__nav {\n    flex: 1 0 100%;\n    font-size: 1.1rem;\n    text-align: left; }\n", ""]);

Minification is a step I'm yet to do but I wanted to get this working before I complicate things!

Again I'm new to webpack but as far as I can tell I have everything working sensibly.

Ah, I've just thought to re-read your earlier question:

do you transpile your scss into css with a Webpack loader? So at runtime, css-resource-plugin is right.

I was wrong with my earlier reply, which may have confused matters (sorry)... yes, I do transpile my scss into css with a webpack loader. But the code added in #12 adds a plugin named scss-resource-plugin for files loaded with an .scss extension. So when the file gets referenced and loaded by aurelia it's loaded with that but the HMR assumes it was loaded with css-resource-plugin.

I don't think I'm doing anything wrong/weird here, I just think this was missed.

Sorry for wall of text.

AdamWillden commented 6 years ago

Having re-read everything again I am definitely doing the method you suggest as the right way of doing things.

jods4 commented 6 years ago

Yeah, I thought about this a bit more as well. I'm not even sure aurelia-webpack-plugin supports client-side loaders.

For now I think that if you use webpack to build your project, you should transform everything at build-time and not use loaders at runtime.

Unless a compelling use-case comes up, I think it's better to leave it at that and not support loaders in aurelia-webpack-loader.

Going back to the initial issue: we need to modify hot-module-reload to not apply loader, right? Because right now your css goes through an automatic scss loader because of the extension?

AdamWillden commented 6 years ago

@jods4 sorry for delayed response. Ignoring the rest (cause it's not important) I'll focus on this:

Going back to the initial issue: we need to modify hot-module-reload to not apply loader, right? Because right now your css goes through an automatic scss loader because of the extension?

I'm not sure what you mean by "to not apply loader" or what you're asking in your following question.

The problem is really simple. As far as I can tell I'm just trying to make it work as you initially intended, you just missed an additional required change. I urge you to look at the proposed fix in #17. The facts are this: