clappr / clappr-level-selector-plugin

Clappr Level Selector Plugin
MIT License
76 stars 56 forks source link

Simplify consuming this with webpack #48

Closed ropez closed 6 years ago

ropez commented 7 years ago

See clappr/clappr-level-selector-plugin#40

kslimani commented 7 years ago

👍 @ropez could you edit this PR and do not modify files in dist folder ? (it may be one reason why it is not merged yet).

ropez commented 7 years ago

@kslimani updated

maxlapshin commented 7 years ago

so only merge this and it will work with webpack?

kslimani commented 7 years ago

@maxlapshin yes it should. Only part i am not sure is the peerDependencies field. Otherwise LGTM. (should work with Webpack, but also Browserify or any node modules compatible)

ropez commented 7 years ago

peerDependencies enures that, when this is installed as a dependency for another package, Clappr must also be a dependency of that package.

Before, with clappr as a dependency:

node_modules/clappr-level-selector-plugin
node_modules/clappr-level-selector-plugin/node_modules/clappr

After, with clappr as a peerDependency:

node_modules/clappr-level-selector-plugin
node_modules/clappr

Yes, this is working with webpack, assuming that the files in dist are updated when is merged. We have been using this branch as a dependency in our project since this PR was sent. (Actually, I think it will break now, since I've reverted the changed in dist)

kslimani commented 7 years ago

@ropez thank you for explanations. Maybe clappr should be set as dev dependency instead ? though, set as peerDependency, at worst it will only download clappr in node_modules folder.

ropez commented 7 years ago

Actually, npm doesn't download peerDependencies, it just prints out a big warning about missing ones.

I guess it makes sense to have Clappr as a devDependency. I just noticed that there is a development setup in public/index.html, which is using Clappr. In that case, webpack-dev-server is also missing.

However, since Clappr is not required for building (and consuming) this library, I don't think it should be a dependency.

kslimani commented 7 years ago

@ropez you are correct. Maybe simply remove Clappr from dependencies and change the clappr url in the demo/dev page to : https://cdn.jsdelivr.net/npm/clappr@latest/dist/clappr.min.js ?

ropez commented 7 years ago

I just remember why we had to remove Clappr from dependencies. Obviously, in our application using the level-selector, we also have Clappr as a dependency. We ended up with two instances of Clappr in our webpack-build bundle:

node_modules/clappr
node_modules/clappr-level-selector-plugin/node_modules/clappr

These instances would conflict, causing strange and buggy behaviour in our application.

kslimani commented 7 years ago

I see 2 scenarios to resolve this issue :

wmucheru commented 6 years ago

Hey guys, is this ready to merge? I was looking for a way to npm install clappr-selector-plugin and this would really help me out

leandromoreira commented 6 years ago

@clappr/core can any of you have a double check?

mgambati commented 6 years ago

any news on this?