-
Notifications
You must be signed in to change notification settings - Fork 37
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:handle version confilct #21
Conversation
Hi @fedeoo Thank you for your PR! :) Is this really an issue? Would you mind adding some tests |
@mastilver I will add some tests soon. |
No it will store only one, the latest one The real issue is that it will use the wrong version for one of the dependencies Can you also get rid of your code around peerDependencies We also need to address the case when: A can't be served through cdn, B and C can
I'm not sure how to fix that yet, feel free to just summit failing tests if you are not sure how to resolve that |
May be it's not a case, Actually we just get a warning when we install those packages. |
There is still a case we should deal with. |
I still think this is an issue, thank you for making me realise it #22 |
This should be dealt by |
@fedeoo Damn, I just realise your approach with If you don't that's fine, I will try to spend some time on it next week :) |
if (this.verbose) { | ||
console.log(`✔️ '${cdnConfig.name}' will be served by ${cdnConfig.url}`); | ||
} | ||
|
||
let result = cdnConfig.var; | ||
if (peerDependencies) { | ||
for (const peerDependencyName in peerDependencies) { |
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 would prefer something like that:
let arePeerDependenciesLoaded = true;
for (const peerDependencyName in peerDependencies) {
if ({}.hasOwnProperty.call(peerDependencies, peerDependencyName)) {
const isLoaded = Boolean(this.addModule(contextPath, peerDependencyName, {env}));
arePeerDependenciesLoaded = arePeerDependenciesLoaded && isLoaded;
}
}
if (!arePeerDependenciesLoaded) {
return false;
}
@fedeoo fixed that on 7257b5f |
Finally release: https://github.com/mastilver/dynamic-cdn-webpack-plugin/releases/tag/v3.3.0 NOTE: the name changed, I hated it... I think it's clearer now! |
Consider the condition that there are different version modules, Only one of them should be resolved correctly.
If we are failed to add the peerDependencies of module A , we shouldn't handle A as a external module.