PepsRyuu/nollup

Issue with solid-refresh plugin and HMR

high1 opened this issue · 15 comments

high1 commented

I'm having issues with solid-js solid-refresh plugin - trying to make HMR work. The plugin works using babel, so I'm adding it to the plugins list, like this:

"development": {
   "plugins": [["solid-refresh/babel", { "bundler": "esm" }]]
},

Nollup starts and compiles everything, but trying to load the page I'm getting:

Uncaught SyntaxError: import.meta may only appear in a module
    3 http://localhost:8080/main.[hash].js:2775
    executeModuleImpl http://localhost:8080/main.[hash].js:230
    resolve_module http://localhost:8080/main.[hash].js:242
    resolve_module http://localhost:8080/main.[hash].js:236
    resolve_module http://localhost:8080/main.[hash].js:234
    _require http://localhost:8080/main.[hash].js:251
    __nollup_entry_exports http://localhost:8080/main.[hash].js:442
    <anonymous> http://localhost:8080/main.[hash].js:444
main.[hash].js:43:34
[HMR] Enabled

I'm not sure what to do at this point - any suggestions? The code is here - https://github.com/high1/solid-typescript-rollup nollup/hmr branch.

Sounds like import.meta is not being transpiled, which is odd. Will investigate!

You need to remove bundler: "esm" from your Babel config. That configuration tells the plugin to use import.meta.hot instead of module.hot.

high1 commented

It works if used in non-esm mode, but although I get HMR notifications, HMR does not refresh the page. I removed module.hot from my config, to make sure that I'm not using Hot Reload. HMR is working with simillar Webpack configuration.

[HMR] Enabled main.[hash].js:284:33
[HMR] Status Change check main.[hash].js:284:33
[HMR] Status Change prepare main.[hash].js:284:33
[HMR] Status Change ready main.[hash].js:284:33
[HMR] Changes Received main.[hash].js:284:33
[HMR] Status Change dispose main.[hash].js:284:33
[HMR] Status Change apply main.[hash].js:284:33
[HMR] Status Change idle main.[hash].js:284:33
[HMR] Status Change check main.[hash].js:284:33
[HMR] Status Change prepare main.[hash].js:284:33
[HMR] Status Change ready main.[hash].js:284:33
[HMR] Changes Received main.[hash].js:284:33
[HMR] Status Change dispose main.[hash].js:284:33
[HMR] Status Change apply main.[hash].js:284:33
[HMR] Status Change idle main.[hash].js:284:33
[HMR] Status Change check main.[hash].js:284:33
[HMR] Status Change prepare main.[hash].js:284:33
[HMR] Status Change ready main.[hash].js:284:33
[HMR] Changes Received main.[hash].js:284:33
[HMR] Status Change dispose main.[hash].js:284:33
[HMR] Status Change apply main.[hash].js:284:33
[HMR] Status Change idle

Not entirely sure what you mean. Seems to work just fine for me. If I modify App.tsx it will do HMR, and if I modify main.tsx it will refresh the page instead.

high1 commented

IHuh - my bad, I commented out Hot Reload for main page, thinking that would also be automated. Is that the proper way, then? Leave Hot Reload for main page and the components will do HMR?
I took another look and I think that the App is just doing reload of page every time - if I just call accept, without reloading, the component does not update.

Calling accept by itself will do nothing. This does differ in behaviour from other bundlers but it is an intentional design choice. Rather than automatically assuming the developer wants to reload, I make the developer explicit opt into that behaviour which feels less ambiguous.

Usually there is no need for a developer to add any code when using a plugin, but what you described is perfectly fine in terms of implementation anyways.

high1 commented

The issue with this is that everything reloads - if I have a nested component, and update only it's state - the parent should not reload it's state, but it does. So, if I'm calling reload from accept, there's no HMR, because everything reloads, and if I remove module.hot.accept, HMR shows in the console, but it's not updating the browser. I have a similar sample, but with Webpack, and there the hot module reload works as it should, and doesn't require additional instructions. The aim is to update this sample to be on par with that one.

Ahhh I see what you mean now. I didn't notice it was actually refreshing the page. Let me have another look.

Found the problem, but not entirely sure what the correct answer is yet. The way that solid-refresh is implemented, there's two issues:

  • It calls module.hot.accept() with the assumption that no callback means it will use a default empty function.
  • It also assumes that module.hot.accept() will do require(module.id) automatically even if not called in handler.
hot.dispose(data => (data.setComp = prev ? prev.setComp : setComp));
hot.accept();

For compatibility with Nollup, it needs to do something like this instead (not the exact code, the Babel transform also needs to be updated):

hot.dispose(data => (data.setComp = prev ? prev.setComp : setComp));
hot.accept(() => require(module.id));

Solid Refresh can be updated to support this, and in theory I don't think it will break Parcel or Webpack. I need to look more into this to see if this is something that Nollup can handle internally, as implementing the assumptions solid-refresh is making could potentially break backwards compatibility for other Nollup compatible HMR solutions. Will update soon.

This PR contains the changes required, but still needs further testing: #214

high1 commented

Thanks, those changes work as expected - seamless HMR :D. Looking forward to #214 being merged!

So unfortunately as suspected, this is a breaking change. The way that the routify project in particular implements HMR for Nollup based projects, it relies on the fact that it can perform cleanup logic in the accept handler and then manually call require when needed. They can update the project to make proper use of the dispose handler, but it would require them updating their code and I know there's a decent number of people who are using their starter kits.

Not entirely sure how to approach this. If I don't implement the change, one project has to update their code. If I do implement the change, another project has to update their code.

Could implement a flag in .nolluprc.js, something like hmrAutoRequire: true to enable this new behaviour, but it seems incredibly unintuitive and awkward to expect developers to know how the HMR of a project is implemented. Another possible solution is that if you call module.hot.accept() without a callback, that it auto-requires, but if you pass a callback, then it doesn't, but that would be inconsistent behaviour with other bundlers still.

https://github.com/PepsRyuu/nollup/pull/214/files

Updated the PR so that if you call accept() without a handler, it will auto-require. This allows it to be compatible with solid-refresh, while maintaining backwards compatibility for existing projects.

After further testing, we're good to go with this compromise approach. Released in 0.18.5.

high1 commented

Thanks a lot!