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

[WIP] feat(hmr) #58

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pankajpatel
Copy link
Member

The initial idea for HMR in storybook

@nogizhopaboroda
Copy link
Member

Looks great to me. Could you pls check if build script works? I am a bit aware of that because of plugins in default config

js/manager.js Outdated
@@ -1 +1,5 @@
require('../components/sandbox-manager/sandbox-manager');

if (module.hot) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need hot reload in manager? Seems like only in preview should be enough

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thats right

@nogizhopaboroda
Copy link
Member

So, should we remove that redundant 3 lines and finally merge it? What about build script? Works?

@pankajpatel
Copy link
Member Author

build script works and the files generated are also fine. though there is a 404 for HMR request

@nogizhopaboroda
Copy link
Member

nogizhopaboroda commented Oct 22, 2018

yep, that's what i was afraid of. seems like it could be fixed by moving hmr stuff (where you extend default config with hmr plugins and dependencies) from lib/get_webpack_config.js to server.js

here https://github.com/modulor-js/modulor-storybook/blob/master/server.js#L44 . Just extend the config before applying it

@nogizhopaboroda
Copy link
Member

any news here? =)

@pankajpatel
Copy link
Member Author

still working on it.

@pankajpatel
Copy link
Member Author

While applying HMR to the components, this error appears. And it is the problem we have no work around as the CustomElementsRegistry does not provide any way to un-define any customElement

error

One way can be to do window frame reload on the HMR update; what do you guys say?

@nogizhopaboroda
Copy link
Member

I think you need to apply hmr only for preview (always reloading main page makes no sense anyway).

To do so, you'll first need to take a solution from my comment above:

seems like it could be fixed by moving hmr stuff (where you extend default config with hmr plugins and dependencies) from lib/get_webpack_config.js to server.js
here https://github.com/modulor-js/modulor-storybook/blob/master/server.js#L44 . Just extend the config before applying it

Then, apply hot middleware only for preview:

app.get('/preview.html', webpackHotMiddleware(compiler));

hope this helps

@pankajpatel
Copy link
Member Author

I did similar already but the change is not on github. The issue is about redefining the component when something is changed.

When we save the component file with changes, the whole file is sent back to preview client and re-evaluated. Now on re-evaluate, it will throw error because the Browser has already defined the component's tag name for that class. And that's what the screenshot is showing.

One way to mitigate this error can be to move all the customElements.define to different file where the re-eval of customElements.define does not happen; similar to global require and attaching to DOM.

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.

2 participants