search

modulor-js / modulor-storybook

3 stars 2 forks source link

[WIP] feat(hmr) #58

Open pankajpatel opened 5 years ago

pankajpatel commented 5 years ago

The initial idea for HMR in storybook

nogizhopaboroda commented 5 years ago

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

nogizhopaboroda commented 5 years ago

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

pankajpatel commented 5 years ago

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

nogizhopaboroda commented 5 years ago

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 commented 5 years ago

any news here? =)

pankajpatel commented 5 years ago

still working on it.

pankajpatel commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.