feat:handle version confilct

[feibyte]

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.

[mastilver]

Hi @fedeoo

Thank you for your PR! :)

Is this really an issue? urls is an object, so if there is a module is required twice, it will be overwritten (Which I agree is not the best solution)

Would you mind adding some tests

[feibyte]

@mastilver I will add some tests soon.
I know urls is an object, but just for a case：A project depends [email protected] and a UI module named A, A depend on [email protected]. we will store both https://unpkg.com/[email protected]/dist/react.min.js and https://unpkg.com/react@^0.14/dist/react.min.js to urls. This may be lead some exceptions.

[mastilver]

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@1 -> B@1
C@1 -> B@2 (peerDependency)
B@2 (because of C)

A can't be served through cdn, B and C can

• A@1 is required: it's being bundled
• B@1 is required by A: it's being served from the cdn
• C@1 is required: it's being served from the cdn / It will use B@1 as it's exposing a global var
• B@2 is required: it's being bundled / it will not be use as C is already using B@1

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

[feibyte]

May be it's not a case, Actually we just get a warning when we install those packages.
I was wrong about the peerDependency. I will close this PR. Thanks for your remand.

[feibyte]

There is still a case we should deal with.
When we are failed to resolve B from CDN, A and C shouldn't be served from the CDN(in most case will be failed as well).

[mastilver]

I still think this is an issue, thank you for making me realise it #22

[mastilver]

When we are failed to resolve B from CDN, A and C shouldn't be served from the CDN(in most case will be failed as well).

This should be dealt by module-to-cdn, there is an issue for that: mastilver/module-to-cdn#5 ;)

[mastilver]

@fedeoo Damn, I just realise your approach with peerDependencies was good, I will put some comments on this PR, if you want to work again on that

If you don't that's fine, I will try to spend some time on it next week :)

[mastilver]

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;
              }

[mastilver]

@fedeoo fixed that on 7257b5f

[feibyte]

@mastilver 👍

[mastilver]

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!