search

solidjs / solid-refresh

MIT License
85 stars 18 forks source link

feat: add moduleName option #47

Open inottn opened 10 months ago

inottn commented 10 months ago

Hope to provide a option that can customize the module name. When this option is not passed in, the default is to use 'solid-refresh'.

lxsmnsyc commented 10 months ago

I'm not sure what problem this solves, can you provide an example?

inottn commented 10 months ago

I'm not sure what problem this solves, can you provide an example?

Refer to this PR, the final solution is to use alias to redirect to the correct path. It would be even better if custom module name could be supported.

lxsmnsyc commented 10 months ago

@inottn I'm not sure I follow, why was alias necessary and not with the package directly?

I would assume it's because of this: https://github.com/inottn/rsbuild/blob/c03334fe5f3c463e63c0eaa0a44628d166e5de44/packages/plugin-solid/src/index.ts#L34-L36

but I would also need to ask why require.resolve is necessary?

inottn commented 10 months ago

@lxsmnsyc If I have any misunderstandings, please feel free to correct me. This plugin does something similar to what vite-plugin-solid does, because users do not directly depend on solid-refresh, so they may not be able to resolve solid-refresh to the correct path during runtime. Therefore, it needs to be handled on the plugin side.

lxsmnsyc commented 10 months ago

because users do not directly depend on solid-refresh, so they may not be able to resolve solid-refresh to the correct path during runtime

this part is the one I'm not sure, since the rsbuild plugin is dependent on solid-refresh, I would assume it would be able to resolve it? on the other hand, I'm not familiar with how rspack resolves modules but have you tried if it would work without the whole require.resolve stuff?

Honestly the only reason we are doing this in Vite is so that we can alias solid-refresh in dev sources (so that it's not too hidden).

inottn commented 10 months ago

@lxsmnsyc Thank you for providing information about Vite. I encountered a module resolution error when I didn't use require.resolve. I suspect this is because the user only installed the plugin and when using pnpm as the package manager, it cannot resolve the path of solid-refresh (as solid-refresh is located in the plugin's node_modules directory). In fact, when I ran pnpm install solid-refresh -D, this issue was resolved. However, it is not a good practice to require users to install both the plugin and solid-refresh at the same time.

lxsmnsyc commented 10 months ago

@inottn One final question, is this line necessary?

https://github.com/inottn/rsbuild/blob/c03334fe5f3c463e63c0eaa0a44628d166e5de44/packages/plugin-solid/src/index.ts#L38-L42

edit: would this also be helpful? https://github.com/pnpm/pnpm/issues/5237#issuecomment-1671383914

inottn commented 10 months ago

@inottn One final question, is this line necessary?

Yes, this is necessary. Thank you for your patient response.

I ran npm create rsbuild@latest and pnpm create rsbuild@latest separately to create a Solid project, then I removed this piece of code from the plugins in node_modules. The project using npm could run normally, but the one using pnpm could not. This is related to the nested node_modules structure of pnpm.

Assume this code can be removed if all users use npm or yarn, because all dependencies will be flattened in the root node_modules.

lxsmnsyc commented 10 months ago

I'll have to take some time to judge this. The thing is this is the only specific case where it doesn't work. Kinda of unexpected because I assume the webpack and rspack demos worked properly