-
-
Notifications
You must be signed in to change notification settings - Fork 198
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
Resolve loaders directly from Encore instead of using their names #739
Conversation
@@ -302,7 +302,7 @@ class ConfigGenerator { | |||
|
|||
rules.push(applyRuleConfigurationCallback('images', { | |||
test: /\.(png|jpg|jpeg|gif|ico|svg|webp)$/, | |||
loader: loaderName, | |||
loader: require.resolve(loaderName), |
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.
This is a nit-pick, but I'm asking in case it's important. loaderName
here may be file-loader
or url-loader
. file-loader
is required by Encore, but url-loader
is something that the user must have installed. What affect does require.resolve()
have on a package that we know the user must have installed?
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.
That's already what we do to check if those kind of packages are installed:
webpack-encore/lib/package-helper.js
Lines 61 to 68 in 134f47a
function isPackageInstalled(packageConfig) { | |
try { | |
require.resolve(packageConfig.name); | |
return true; | |
} catch (e) { | |
return false; | |
} | |
} |
Also, for the Encore 1.0 release (with Webpack 5), I have been wondering if we should consider to stop the practice of requiring things on behalf of our users. We would still, of course, require things we use directly - e.g. I'd love any thoughts you have on that - the goal would be to create less problems (as sometimes requiring these dependencies like we are causes issues) not more ;) |
Will this help with yarn's PnP mode too ? |
@weaverryan with your proposal, a nice effect could also be to reduce the dependency footprint of Encore. For instance, I'm only bundling JS with encore, not CSS, so I don't need all the css-loader or optimize-css-assets-webpack-plugin stuff. |
Thank you @Lyrkan! |
@weaverryan I mean we could, it's probably only a matter of moving them to peer dependencies and adding the features checks in
Also one annoying point is that the current "enforce_version" message is sometimes hidden because it appears after the build summary, which means that users could miss a required update that would have been done transparently in the current version. @stof The issue with doing that is that we can't know before running Webpack if the user need those loaders/plugins (which is the case for other package checks since we do them when building the webpack config). That could probably be handled by looking at the errors thrown by Webpack but it means that someone initializing a project that uses CSS will have to:
I feel that it will annoy people more than downloading something that has a slightly bigger footprint. |
This issue fixes #727 by replacing the use of loader names in rules (for instance
'file-loader'
) by their resolved path (require.resolve('file-loader')
).The problem with using their name is that we are assuming that the loader's package will be present at the top-level of
node_modules
.If for some reason that's not the case and npm/Yarn decides to put it in, let's say
node_modules/@symfony/webpack-encore/node_modules
, the compilation will fail when Webpack tries to require it.Another reason to do that is to ensure that Webpack uses the right version of a loader. We had some issues before 0.29 because we expected an old version of
file-loader
when some other packages included a newer one that was then used by Webpack because it got hoisted to the top-levelnode_modules
.This change should in my opinion be considered a BC break for people that were:
package.json
: that one won't be used anymoreI also modified dev dependencies loaders for more consistency but it doesn't really matter for those.