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

Issue with solid-refresh plugin and HMR #213

Closed
high1 opened this issue Sep 14, 2021 · 15 comments
Closed

Issue with solid-refresh plugin and HMR #213

high1 opened this issue Sep 14, 2021 · 15 comments

Comments

@high1
Copy link

high1 commented Sep 14, 2021

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.

@PepsRyuu
Copy link
Owner

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

@PepsRyuu
Copy link
Owner

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
Copy link
Author

high1 commented Sep 14, 2021

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

@PepsRyuu
Copy link
Owner

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
Copy link
Author

high1 commented Sep 14, 2021

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.

@PepsRyuu
Copy link
Owner

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
Copy link
Author

high1 commented Sep 15, 2021

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.

@PepsRyuu
Copy link
Owner

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

@PepsRyuu
Copy link
Owner

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.

@PepsRyuu
Copy link
Owner

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

@high1
Copy link
Author

high1 commented Sep 16, 2021

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

@PepsRyuu
Copy link
Owner

PepsRyuu commented Sep 16, 2021

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.

@PepsRyuu
Copy link
Owner

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.

@PepsRyuu
Copy link
Owner

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

@high1
Copy link
Author

high1 commented Sep 17, 2021

Thanks a lot!

@high1 high1 closed this as completed Sep 17, 2021
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

No branches or pull requests

2 participants