-
-
Notifications
You must be signed in to change notification settings - Fork 184
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
hot module reload #125
Comments
Should I send PR? |
Current implementation const getFelaMountNode = () => {
const node = document.getElementById('stylesheet');
const parent = node.parentNode;
if (!node || !parent) {
throw new Error('missing stylesheet node for Fela');
}
const nextNode = document.createElement('style');
nextNode.id = 'stylesheet';
parent.replaceChild(nextNode, node);
return nextNode;
}; |
Where exactly would you add that? I never used hot reloading in any of my projects as I am using another approach to do stateful live devopment. I'd be really happy to officially support hot reloading with Fela! |
In the Este uses native (without plugins as recommended by Dan Abramov when app doesn't use local state) HMR. https://github.com/este/este/blob/master/src/browser/main.js#L31 |
For Fela it means, no private state in modules! When file is changed, the whole app is restarted, which is ok and stable, if app state is in Redux. |
Perhaps the best way for me to understand it, is to just show me all changes. It sounds kind of what I am using for a current project, but it's even more "dumb" I guess. I am syncing my whole Redux store to localStorage every single time it changes and intially load the store from localStorage as well. For remote development (e.g. mobile) or other browsers I use a simple server that syncs the store globally. It's quite helpful to have persistent development, but it needs to reload the page every time. Also any state needs to be part of Redux^^ |
localStorage is ok, stateless modules as well, but stateful js modules are tricky, must be handled like that https://github.com/este/este/blob/885358e7a78c59df3a7a7a3b218456194b63111b/src/common/configureMiddleware.js#L37 |
I will send you a PR. |
Hot reloading worked fine for me. I'm not sure what would need to change in the project, seems to be implementation specific. // Initial stylesheet w/ global styles
const renderer = createRenderer();
const createGlobalStyles = rules =>
Object.keys(rules).forEach(selector =>
renderer.renderStatic(rules[selector], selector));
createGlobalStyles(globalStyles);
// Initial render.
ReactDOM.render(
<Fela renderer={renderer} mountNode={mountNode}>
<Root store={store} />
</Fela>
, appElement);
// Hot reload render.
// gist.github.com/gaearon/06bd9e2223556cb0d841#file-naive-js
if (module.hot && typeof module.hot.accept === 'function') {
module.hot.accept('./app/themes', () => {
renderer.clear();
createGlobalStyles(require('./app/themes').globalStyles);
});
module.hot.accept('./app/Root', () => {
const NextRoot = require('./app/Root').default;
ReactDOM.render(
<NextRoot store={store} />
, appElement);
});
} |
It works for me as well. Still I would rather: /* @flow */
import App from './App';
import React from 'react';
import configureFela from '../../common/configureFela';
import { BrowserRouter } from 'react-router';
import { Provider as Fela } from 'react-fela';
import { Provider as Redux } from 'react-redux';
type Props = {
store: Object,
};
// This should be part of Fela.
// TODO: https://github.com/rofrischmann/fela/issues/125
const getFelaMountNode = () => {
const node = document.getElementById('stylesheet');
const parent = node.parentNode;
if (!node || !parent) {
throw new Error('missing stylesheet node for Fela');
}
const nextNode = document.createElement('style');
nextNode.id = 'stylesheet';
parent.replaceChild(nextNode, node);
return nextNode;
};
// We needs such Root for vanilla hot reloading.
const Root = ({ store }: Props) => (
<Redux store={store}>
<Fela mountNode={getFelaMountNode()} renderer={configureFela()}>
<BrowserRouter>
<App />
</BrowserRouter>
</Fela>
</Redux>
);
export default Root; But it has a low priority. |
I can't even get it to work properly, but that might be because I'm using code splitting... UPDATE @steida I wonder why tt worked for you with without this... |
Anything we can do at Fela, to improve the hot reloading experience? |
@rofrischmann I believe this belongs to Fela https://github.com/este/este/blob/c7e1138e51be6a8c27ba534dc8ecd0c74a695a57/src/browser/app/Root.js#L13 As for hot reloading experience, we don't need anything else. Este uses the most frictionless hot reloading, just rerendering everything. It works because whole state is in the Redux anyway. That's why I have to call getFelaMountNode(), because app was rerendered because hot reload. What do you think? |
I don't think this should be part of Fela. I'm creating an app that renders into <iframe>. So stylesheet need to be passed down to the child document of the page. If fela will handle styleshhet node like this, it will be disaster for this kind of applications. |
I also don't see this being part of Fela at all, but we should add instructions to the documentation on how to actually enable hot module replacement with Fela. Any thoughts are welcome. |
For anyone still looking for how to do hot-reloading with webpack, this is how I did it: Root.js import getRenderer from '../lib/css-renderer'
import { render } from 'fela-dom'
if (module.hot) {
module.hot.dispose(() => {
const renderer = getRenderer()
renderer.clear()
render(renderer)
})
} lib/css-renderer.js import { createRenderer } from 'fela'
const renderer = createRenderer()
const getRenderer = () => {
applyGlobalStyles(renderer)
return renderer
}
export default getRenderer |
@queckezz can you add Thank you. |
@rofrischmann I don't see any docs on how to get fela to work with hot module reloading. Does anyone have any guidance on how to get it to work with storybook?? |
Hi @rofrischmann I was digging through the docs to see if I could find an example for hot module reloading as referenced in this comment but I was not able to find anything except this closed issue. Was this example removed in a previous version perhaps? |
Need to enable |
This works but it's not probably a good approach.
Why do we have to reset textContent on the server-side rendering? It looks like an implementation detail. Also, removeAttribute is probably wrong, because subscribing. Should I create empty node manually?
Note Root is render on module change in development.
The text was updated successfully, but these errors were encountered: