-
Notifications
You must be signed in to change notification settings - Fork 18
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
Troubleshooting steps to make HMR work in complex webpack configs #68
base: main
Are you sure you want to change the base?
Conversation
README.md
Outdated
}, | ||
resolve: { | ||
conditionNames: prod ? ['browser', 'import'] : ['development', 'browser', 'import'], | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually for me without specifying conditionNames, hmr works. So it's probably not good to include that in the README if it's not needed by default.
["import", "module", "..."]
seems the default: https://github.com/webpack/webpack/blob/ac8e83a4a2984ed977fd6c23a609da5cb43da492/lib/config/defaults.js#L1506-L1518
Including "..." seems to be needed for hmr to work, I don't know exactly the meaning of it.
I guess you had conditionNames set to force using 'browser' key and hmr didn't work so you added 'development' explicitly in the list?
It's maybe better for you to set mainFields option? https://webpack.js.org/configuration/resolve/#resolvemainfields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for investigation. 👍
I've tried to add solid to a svelte project to gradually migrate, and svelte suggests adding itself to both mainFields
and conditionNames
, which look something like this in this project:
mainFields: ['svelte', 'browser', 'module', 'main'],
conditionNames: ['svelte', 'browser', 'import'],
Removing them both indeed made hmr work out of the box, so you are right, it's not needed by default.
I guess we could add a troubleshooting step for webpack configs further down in the readme instead? 🤔
It feels unrelated to solid-refresh
though, so I'm not sure. 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we could add a simple note like "If you modified the conditionNames
option, be sure you have '...'
in the list to include the default values for hmr to work properly. For example if you have svelte and solidjs in the same repository sharing a webpack config, you can use conditionNames: ['svelte', 'browser', '...']
" (not sure if 'browser' is also needed for svelte?)
I just asked gpt-4o about it:
In Webpack's configuration, the ...
syntax used within arrays for options like conditionNames
has a special meaning. Specifically, it is an "extension" operator that indicates Webpack should include the default values for that array in addition to any custom values you specify.
Here’s how it works in the context of conditionNames
:
- The default value for
conditionNames
is["import", "module"]
. - When you use
["import", "module", "..."]
, it means "use the default values (which areimport
andmodule
), plus any additional conditions specified before the...
".
If you want to add a new condition without losing the default ones, you can use the ...
to ensure they are included. For example:
module.exports = {
// Other webpack configuration...
resolve: {
conditionNames: ["import", "module", "custom", "..."]
}
};
In this configuration, conditionNames
will effectively be ["import", "module", "custom", "import", "module"]
, where the ...
is replaced by the default conditionNames
. However, duplicates are usually ignored or handled, so it will generally be interpreted as ["import", "module", "custom"]
.
This is useful because it allows you to customize the configuration while still keeping the default behavior intact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot! 🎉
I've checked it and it works.
Let me know what you think about this patch. 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@non25 you have svelte
in your new commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vincentfretin I meant to say that everything that is supposed to be Solid is svelte
. You could add solid
too (like what we do for the Vite plugin), but I'm not sure if this (solid-refresh) is the correct place to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So solid packages, that require JSX transpilation could be consumed either as js files, or untranspiled JSX? 🤔
With svelte you'd have two runtimes, which is critical.
What is drawback for using transpiled JSX from npm packages for solid? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For mainFields and conditionNames there is no need to put 'solid'. I don't have this currently in my config.
For some dependencies like solid-icons I have a rule to run solid babel plugin yeah, see https://github.com/networked-aframe/naf-valid-avatars/blob/main/webpack.config.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@non25 the drawback is it cannot utilize the SSR/client build properly, and the builds aren't consistent. We usually recommend shipping with preserved JSX as the main source
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lxsmnsyc then I think we need to add resolve.mainFields
and resolve.conditionNames
with solid as required addition to the webpack config. Otherwise using 3rd party libraries even in CSR is not ideal. 🤔
I'll add if you agree.
d18861b
to
fe16735
Compare
fe16735
to
937de6e
Compare
Hello!
I've spent some time figuring out why it didn't work for me in webpack 5.
And it appears that you need to force webpack to use
development
export condition.The simpler way to do this is to use resolve.conditionNames.
The complicated way to do this, which I used initially is to force paths with aliases:
So here's slight readme adjustment to hint that to avoid some confused head-scatching for someone else. 😅
Thanks for the plugin! Works real good. 😁 Even render-props work, wow. 🎉