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

feat: new plugin api with integrated HMR loader #146

Merged
merged 4 commits into from
Mar 4, 2019

Conversation

ScriptedAlchemy
Copy link
Collaborator

under the advisement of webpack engineers, API has been re-written to include the hot module replacement loader directly into the CSS loader itself. The new API has brought drastic stability to the plugin

BREAKING CHANGE: New loader API and options.

@ScriptedAlchemy ScriptedAlchemy merged commit fd6d288 into master Mar 4, 2019
@faceyspacey
Copy link
Owner

🎉 This PR is included in version 4.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@glebtv
Copy link

glebtv commented Mar 6, 2019

This breaks hot reload on my projects, it tries to reload but all I get is [HMR] Nothing hot updated. and css is not updated, with or without reloadAll (version 3 works fine for me)

Any info on how to update from 3 to 4?

@ScriptedAlchemy
Copy link
Collaborator Author

Check the readme. Options moved from the plugin to the loader itsself.

Instead of defining hot and reload all on the plugin. Create loader options

@martiuh
Copy link

martiuh commented Mar 8, 2019

I'm having the same problem as @glebtv, I already changed my config to the loader. I'm going to do a minimal repo to test it and share it.

@ScriptedAlchemy
Copy link
Collaborator Author

ScriptedAlchemy commented Mar 8, 2019

Yeah please do, this was a major change and a pretty difficult refactor. So far I’ve had about 40k people update without much issue so I’m hoping everything has gone smoothly. Will appreciate a minimal example and will prioritize a patch. This project is merging with mini-css-extract. So if we have any bugs in the new update (which was done for webpack), I’d want to catch them now before the user base grows by several million

@martiuh
Copy link

martiuh commented Mar 8, 2019

@ScriptedAlchemy first of all thanks for the great work you've done with the universal family modules, I'm a huge fan, secondly here's the repo I made, it's using the react@16.8.4 and the correspondant react-dom also it has extract-css-chunks-webpack-plugin@4.0.0

(I did try @hot-loader/react-dom but it still doesn't work)

git clone git@github.com:martiuh/minimal-non-hmr-extract-css-chunks.git
cd minimal-non-hmr-extract-css-chunks
yarn
yarn start

There you can see how HMR is not workin on a non-imported component (Navbar.jsx) and you can test it aswell with three imported components (Home.js, NotFound.jsx and About.jsx), I thought it had something to do with sass-loader but It's also not working with pure css-loader. Hope this can help find something hidden.

@ScriptedAlchemy
Copy link
Collaborator Author

I really appreciate the praise :) it still is incrediblely heartwarming to see these tools be so helpful to others.

Hmm 🤔 I can’t spot anything wrong with your config, at least off my phone. I do like your use of webpack merge!

I’ll pull this down over the weekend and see what’s going on. Can you try enabling the modules:true option (shouldn’t have to but if it works then there’s another bug)

@ScriptedAlchemy ScriptedAlchemy deleted the v4-mini-css-proposal branch March 8, 2019 20:44
@martiuh
Copy link

martiuh commented Mar 8, 2019

I did enable modules: true and HMR did work. I created a new branch use-css-modules where you can see that it does work with this almost identical setup.

Hope this helps and if you need anything else please just ask and I'm going to try do my best.

@ScriptedAlchemy
Copy link
Collaborator Author

Okay so to confirm enabling modules does solve the problem.

Without looking at your repo, again on a phone. You are not using css modules (imprting stylesheets as a variable and referencing classes as if they were objects)

If enabling module for non-modules based files works for you. Then I’ve actually fixed a long standing problem 😂 which will let me remove the modules option all together

@martiuh
Copy link

martiuh commented Mar 8, 2019

That'd be great but not what I meant 😆, sorry for not being clear.

What I meant was that using modules: true and importing stylesheets as variables
import styles from '../css/Navbar'
does HMR

After I read your response I did try however enabling modules on ExtractCssPlugin.loader, but importing to the global scope (I think that's how it's called)
import '../css/Navbar' but did not HMR

I also tried multiple combiantions for the ExtractCssPlugin.loader setting like { hot: true, reload: false, modules: true } and such but did not get anything promising.

@ScriptedAlchemy
Copy link
Collaborator Author

Okay so css modules work but global scope doesn’t.

And you’re not mixing global and local scoped modules?

Damn, I’ll look into what’s wrong with the loader. I’d stick with version 3 for convenience till I can resolve this

Sorry about that!

@martiuh
Copy link

martiuh commented Mar 9, 2019

  1. That's right, css modules does work and global doesn't
  2. I'm not mixing, I tried though in order to see if something happens, but nope.

Sure if I can help please let me know although I'm not as experienced as yourself, and I just downgraded to 3.3 to keep working on my app, nothing to be sorry about I understand completely and hope I can help.

@ScriptedAlchemy
Copy link
Collaborator Author

Okay, Ive got a pretty good idea of where to look. When i find the problem, ill try and post some code and walk through whats going on.

This loader and plugin, its really cool in terms of how much you can do with webpack. I didn't write all of it, its based on mini-css originally. But its a super modern loader/plugin combo thats simple enough to follow along to. Throwing debuggers and console logs in various methods reveals a huge amount about what you can do with webpack.

Without looking at the code right now. Here's my diagnosis - you see we have a hot loader file, in there i have something that checks for css modules and hot on a query parameter. Thats how I'm able to pass data between plugin and loader. From what i know, loaders and plugins cannot really communicate with each other easily and pretty much leaving a query string on one end lets us pick it apart on the other. V3 circumvents this with a huge hack, mutating the original webpack config and just adding an entire loader on after in the constructor - then evaluating the whole webpack object. It was super effective but depended on hard-coded assumptions

So whats going on? In the main file ( i think ), theres a comment like "extracted by ..." thats where a little shred of data is chained onto the end. So what i think is happening is that its only chaining it properly when theres JS involved to start with (css modules, which create another little hash map file).

The bug lays in either the hot loader who's query string or conditional isn't getting the right info, or in the main file, right below where "extracted by" comment is-where we actually append the hot loaders query string and context.

What may need to happen - if its not a css module, then the context should be provided for the parent file/bundle, where we would be adding the extra HMR part to. The other alternative is that we just need to generate one additional JS file that actually contains the "rules" for hot reloading, when webpack reloads the JS, it contains the hot reloading properties which would in turn update the stylesheet. This is more or less what happens now, the css HMR is very un-webpack once the script actually runs in the browser. Like something you'd see in the jquery world haha, appending links and such.

Helping family move today, but should get to it tomorrow if or hopefully tonight. It's tricky to navigate, so even though ive got a good idea where the problem is. Debugging these things kinda sucks and lacks dev tools. Ill likely pull two copies of your repo, one on v3 and one on v4, then place debuggers in various locations along the hot loader to see what data is actually being sent back during global css reloads (what file, context, query)

Hopefully that helps shed some light on how things work. Ill admit that webpack loader development is confusing until it clicks - reminds me a lot of early experiences with redux where its just hard to conceptualize, but once you know how the black box works- things become more predictable and make sense.

If you ever have the time, I'd totally recommend just playing around with the mini-css or extract-css-chunks project. Putting various logs in places to see what goes in and what comes out. You can learn a huge amount about webpack. It's been a very profitable skill in the enterprise architecture arena.

Judging from your WP configuration, you know webpack better than most engineers ive encountered professionally. The ability to debug some random error that gets thrown has brought many to a grinding halt.

@martiuh
Copy link

martiuh commented Mar 10, 2019

Wow!!! Thank's for that, it's pretty cool and useful. I can totally relate to the redux's example.
I'll definitely dig into that code to learn more of webpack's functionality and hopefully solve the bug. also, I appreciate the webpack plugin debugging tips.

@ScriptedAlchemy
Copy link
Collaborator Author

Okay, From what I can tell, I fixed it.

Very stupid mistake, as i detailed above it was that conditional on the "extracted by" comment in the main file.

What happened?

In loader.js - I have the following little bit of code

    let resultSource = `// extracted by ${pluginName}`;
    if (locals && typeof resultSource !== 'undefined') {
      const result = `\nmodule.exports = ${JSON.stringify(locals)};`;
      resultSource += query.hot ? hotLoader(result, { context: this.context, query }) : '';
    }

locals usually is the hash map for css modules. resultSource is always defined so that's useless.. lol.

Pretty much it has to stringify the hashmap (nothing special) to write the code that correlates your imported classes with the local scoped ones. So obviously webpack is going to complain if I module exports undefined which is why the conditional was there... Problem is, as you probably see - if there's no locals then no hotLoader is getting added.

Removing the conditional and just having result return as an empty string when locals is undefined fixes it. Heres the pull request, i have also published a @next tag on npm for you to test #150

How did I know where to look?
You should have noticed, the browser console did actually say a few things about HMR - at least on the first attempted hot reload. I saw home.js noted and if you check the network tab you'll see a home.js.hot.update file (or something) and in there it did indeed specify some information that was accurate. So i knew that the plugin and all other aspects were working - it was just a case that the code wasn't making it the last stretch, leading me right to the hot loader which didn't give any new context (because it was never even run)

Let me know if the beta on npm works for you, if so ill merge it

@zacharyrs
Copy link

Hey,

Just wanted to chime in and say I was facing the same issue after upgrading.

I tested the build @next, and it seems to be working again!

@ScriptedAlchemy
Copy link
Collaborator Author

Awesome thanks everyone! Sorry for the issue

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

Successfully merging this pull request may close these issues.

5 participants