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

feat: add moduleName option #47

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

inottn
Copy link

@inottn inottn commented Nov 29, 2023

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

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

@inottn
Copy link
Author

inottn commented Nov 29, 2023

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

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

inottn commented Nov 29, 2023

@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
Copy link
Member

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

inottn commented Nov 29, 2023

@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
Copy link
Member

lxsmnsyc commented Nov 29, 2023

@inottn
Copy link
Author

inottn commented Nov 29, 2023

@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
Copy link
Member

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

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

Successfully merging this pull request may close these issues.

2 participants