search

solidjs / solid-refresh

MIT License
85 stars 18 forks source link

Pragma is not detected if it's not the first comment in file #22

Closed victor-wm closed 5 months ago

victor-wm commented 2 years ago

Hello!

We have encountered a situation where the /* @refresh reload */ pragma is not respected because it's not the first comment in the file.

It's particularly tricky because our files have a copyright header which is added automatically, meaning we need /* @refresh reload */ to appear after the copyright.

In contrast, both Typescript and ESLint pragmas work fine after the copyright header. i would expect solid-refresh to be the same.

Example:

// This is a copyright header
// which is automatically injected

/* @refresh reload */

const App = (props) => {
...
}
lxsmnsyc commented 2 years ago

Weird that it should work given that the plugin checks for every known comment in the file. What version of solid-refresh are you using?

victor-wm commented 2 years ago

I'm using 0.4.1.

Not sure if there's something in my setup that makes it work differently but I'm using Webpack and the solid preset points to generate universal.

After some investigation I noticed it doesn't work for the App.tsx but it does work if I place the pragma in another component. So right now the place I see it not working is my root component.

I will investigate it a bit more.

CreativeTechGuy commented 8 months ago

I can confirm what @victor-wm said is still an issue. And it looks like they are onto something. If the comment is in the entry point file it doesn't work. But if you move it one file deeper then it works as expected.

lxsmnsyc commented 8 months ago

@CreativeTechGuy I cannot reproduce this anymore in the latest Refresh, could you provide more details on the setup you're using?

CreativeTechGuy commented 8 months ago

@lxsmnsyc Here's a minimal repo reproducing the issue. With the latest versions of the dependent packages.

https://github.com/CreativeTechGuy/solid-refresh-pragma-repro

If the latest version of solid-refresh is working, then maybe the solution is that the other solid packages (like vite-plugin-solid) which depend on it be updated also. 🤷‍♂️

CreativeTechGuy commented 7 months ago

Hi @lxsmnsyc . Were you able to take a look at the repo I linked demonstrating this issue?

CreativeTechGuy commented 6 months ago

Hey just checking in @lxsmnsyc were you able to take a look at my reproduction?

lxsmnsyc commented 6 months ago

@CreativeTechGuy I've tested it and I can only conclude one summary: this behavior is undefined.

The compiled code is correct, the runtime implementation is correct, and the HMR logs are correct. However, the reason it's undefined behavior is because Vite is expected to reload when the root module invalidates, but it didn't. There's no description of how invalidate is supposed to work (in the docs) so it's pretty much undescribed. Whether or not this is intended is unknown.

lxsmnsyc commented 6 months ago

You know what, I'll just use a different output for this one.