Few Questions

[HenriBeck]

I have a few questions about the code.

Do we need this check, because the user should only add the plugin in the dev config anyway? This should remove the need for the ternary.

Is using the name here a good idea? Because when two components have the same name, we got a problem.

Also, I'm not a fan of the inline function in the ternary.

[felthy]

Hi @HenriBeck thanks for reviewing! 🙂

1. @kof also commented on that check and I plan to remove it and, instead, log an error to the console if the code happens to be executed in production.
2. Yes I’m not 100% happy with that either, but we need some way to track when we’re dealing with a newly hot-replaced version of the same HOC. Object identity is no good. react-hot-loader uses a babel plugin to generate a unique id based on things like the filename, but we can’t access that information from here. I decided to go with this to get it working but we should try to find a better way. Open to suggestions here!

[HenriBeck]

Can't we use the reference to the InnerComponent? The component should only ever be wrapped once and the reference doesn't change when an HMR updates the pages, right?

[kof]

Mb we should rewrite injectSheet in a way that allows implementing HMR from the outside without making injectSheet implement specific logic for HMR, but instead more generic stuff.

Since injectSheet hoc is all about managing side effects of CSS, maybe we need to abstract away the side effects and allow some control of those from the outside.

Basically we need to provide a couple of hooks which can be passed over props from the HOC and receive required references.

Those hooks could be non-documented, so we don't bloat the public api surface, but we could still clearly separate those components

@felthy can you make a suggestion here about what hooks would you need and what arguments should they receive?

[HenriBeck]

I think we would just need a way to update the state when the props change? So may a function which will return whether or not to update the sheets? And in react-jss we could just default that prop to the normal implementation.

[kof]

One more reason to keep HMR logic in a separate module: after all you might not need it soon or it might be much simpler https://twitter.com/dan_abramov/status/1016994041823498241

[felthy]

The reference to the InnerComponent didn't work. I think that's because it is passed in prior to being wrapped, but it's pretty hard to understand what's going on to be honest. The HOC itself gets proxied by react-hot-loader as well though, so I can use Object.getPrototypeOf(this) within instances as the key! I've just published a new version 0.1.2 which does this, so we can handle components with the same name now.

@kof I'm still thinking about how to separate this properly, it's pretty hard to reason about though. I haven't given up!

[kof]

@felthy chat with me on gitter or twitter if you want to talk about it faster

[HenriBeck]

I think we might need something like react-redux connectAdvanced function.

[kof]

@HenriBeck what do you mean exactly?

[HenriBeck]

react-redux connect function uses the connectAdvacned function. You can think about the connectAdvanced function to be like the createHOC function. The function just has a few more customizability options. We could update the createHOC function to accept things like a function for determining whether or not to update. Then we could simply just export a similar function from the webpack function which considers hot reloading as well.

Also wanted to throw in the way react-redux does HMR: https://github.com/reduxjs/react-redux/blob/master/src/components/connectAdvanced.js#L258

So to sum up:

The createHOC would get additional arguments:

• shouldUpdateSheet (Whether or not to unmanage the sheet and create a new one)
• And we might need some callbacks for when things happen

[kof]

I found out about this issue: gaearon/react-hot-loader#1024 Mb we should just keep it that way as it is right now and not reinvest into HMR at all

mui/material-ui#12160

[felthy]

That’s intriguing ... but a year is a long time!

Personally, I wouldn’t care much about HMR if it wasn’t for JSS - JSS is the only place where I want (need?) it. The kind of work I do is pretty heavy on the CSS, and building styles from scratch with injectSheet() is super tedious without HMR. In the olden days I used Sass with browsersync, and hot style replacement is the only thing I miss about doing development that way.

Having said that, I do agree that keeping this the way it is (i.e. not rearchitecting react-jss for the benefit of HMR) is the right thing to do right now. It’s good enough. Perhaps I should add a warning to the readme, to the effect that this package is liable to break at any time due to changes in react-jss or even in react-hot-loader, so beware. The consequences are minimal and easy to spot, and don’t affect production code, so I think that’s a fair tradeoff.

[kof]

Are you really relying on the current state or you just need the webpack hot reload?

[kof]

Yeah and also evtl its good to have that hmr issue linked for people thinking hmr is fine.

[felthy]

The two main difficulties seem to be:

1. after HMR, the SheetsManager that held the refs to the old sheet(s) is cast adrift, so even if the HOC knew to recreate the sheets, it wouldn’t be able to detach the old sheets because it no longer holds a reference to the old SheetsManager instance
2. during HMR, the HOC constructor regenerates classnames, but soon afterwards RHL restores the pre-HMR state for that HOC instance - so the classnames held in the HOC state do not match those in the sheet held by the (new) SheetsManager

So I suppose 2 is only a problem because of RHL. But 1 would be a problem regardless.

[kof]

So I suppose 2 is only a problem because of RHL. But 1 would be a problem regardless.

Regular webpack hot reload just reloads the entire page, why would 1 be still a problem?

[felthy]

I mean if I’ve implemented module.hot.accept(), so I’ll be re-require()ing a React component’s module without reloading the entire page, so createHoc() is being called again without a page reload.

[kof]

I wonder why not to change styles in dev tools if instant feedback with current state is required. I guess its just all about habbits and preffered ways of editing.

[felthy]

Yes I do use dev tools like that. But if I’ve changed a few different rules in the dev tools, switching back and forth between the browser and the editor to copy-paste things isn't possible because the editor’s auto-save causes the browser to refresh and lose my changes.

With two screens, changing things directly in the editor on one screen and seeing the changes in the browser on the other screen means I don't have that copy-paste step between the dev tools and the editor.

[felthy]

I was wrong about 1 being a problem with vanilla HMR - because if you re-render the entire React component tree, the proper component lifecycle occurs and JSS does clean up. Wow!

So without react-hot-loader I can just do this at the top level:

const render = App => <ReduxProvider store={store}>
  <JssProvider>
    <App />
  </JssProvider>
</ReduxProvider>

ReactDOM.hydrate(render(App), rootEl, () => {
  ...
})

if (module.hot) {
  module.hot.accept('./App', function () {
    const App = require('./App').default
    ReactDOM.render(render(App), rootEl)
  })
}

And it works. It works with or without ThemeProvider and with or without isThemingEnabled (i.e. passing an object or a function to injectSheet()).

And I don’t need this package at all.

[kof]

I thought webpack hot reload just reloads entire page or are you doing something else right now?

[felthy]

By default yes, the client in webpack-dev-server will reload the entire page when a hot update is received. The reason that happens though, is that the update was not accepted by any accept handlers. If you do implement an accept handler, like the one in my previous comment, then all updates accepted by it do not trigger a page reload.

That’s all I ever needed - but it didn’t work for me when I first tried because I forgot to delete this from my webpack configuration:

  {
    ...
    loader: require.resolve('babel-loader'),
    options: {
      plugins: ['react-hot-loader/babel'],
    },
  }

That's enough to stop it working, because it generates proxies for my components so React does an update instead of unmounting and remounting. Ugh.

Moving forward, this package would still be useful for users of react-hot-loader, so what I think I should do is:

• Write an HMR section in the react-jss readme, where I strongly recommend the vanilla webpack HMR approach and explain how to set it up, and I’ll point out that the RHL babel plugin breaks the vanilla setup; I’ll also refer to this package for users who require RHL. Do you mind if I use your PR cssinjs/react-jss#275 for this @HenriBeck?
• Update the readme in this package to advise against using it, and refer back to the HMR section in the react-jss readme

Does that sound reasonable @kof?

[felthy]

Also @HenriBeck I’ve fixed the things you originally mentioned at the beginning of this issue, are they resolved to your satisfaction?

[HenriBeck]

Yes, looks good to me👍

[kof]

@felthy this plan sounds reasonable! Lest do this!