clappr / clappr-level-selector-plugin

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

How do I include this as a dependency when using webpack to build my app? #40

Open ropez opened 8 years ago

ropez commented 8 years ago

With reference to this issue in the webpack project: https://github.com/webpack/webpack/issues/1570

Since I didn't find a package in the NPM registry, I included this in my package by referencing the Github repository directly:

"dependencies": {
   ...
    "level-selector": "git+https://github.com/clappr/clappr-level-selector-plugin.git",
   ...

The first issue I found was that it failed to import Clappr, because in my node_modules, its spelled clappr without the capitalization. After having resolved that, I have several issues with .scss and .html files not being loaded correctly. I suppose I have to copy your entire module.loaders section into my webpack config, and see how that goes.

So, do you have any experience or thoughts about how to do this? Frankly, I don't know how to do this properly. That's why I opened the issue with webpack devs, but the issue was closed and we were asked to report it "upstream" (by which I assume they mean here).

leandromoreira commented 8 years ago

@tjenkinson do you know how to help? ( <- he's skillful with this)

tjenkinson commented 8 years ago

This is the same issue that I mention in this PR: https://github.com/clappr/clappr/pull/1115

So if the above PR is merged and 'Clappr' changed to 'clappr' in https://github.com/clappr/clappr-level-selector-plugin/blob/master/webpack.config.js it should work.

You shouldn't need to copy the loaders but in your build process you will need to copy the asset files from 'node_modules/clappr/dist' to your public folder for your app.

If you want to build clappr yourself then you need to tweak your webpack config to point at the clappr src directory not dist, and then you will need to copy the loaders config.

rictorres commented 7 years ago

Apparently that PR didn't solve this issue (at least for us at mycujoo.tv).

A temporary* workaround was to create a gist with the fix and:

"dependencies": {
   ...
    "level-selector": "git+https://gist.github.com/rictorres/169b27559522aaafd9b78e3263b89fc3.git",
   ...
}

*definitive 😎

ropez commented 7 years ago

@rictorres I tried your gist. I get this error:

Uncaught SyntaxError: Unexpected token export

when trying to import like this:

import LevelSelector from 'level-selector';

ropez commented 7 years ago

@rictorres My mistake, I forgot to run npm install, I just updated package.json and restarted webpack. It does work.

ropez commented 7 years ago

Wrong again. The gist makes the modules clappr and level-selector available for import, without any workaround in my webpack config. However, it doesn't resolve the problem that I end up with two instances of the clappr module in my bundle. When level-selector imports clappr, it gets a different instance than the one my app gets, and this causes the browser to freeze in some cases.


update:

$ npm ls clappr
├── clappr@0.2.63 
└─┬ level-selector@0.1.10 (git+https://gist.github.com/169b27559522aaafd9b78e3263b89fc3.git#e9311e131ba0ee6433412384c564e9da7e17f8d3)
  └── clappr@0.2.63 
$ grep "Your browser does not support the playback of this video" build/app.js | wc -l
2
ropez commented 7 years ago

I created a new gist where I changed the dependencies to peerDependencies: https://gist.github.com/ropez/f6c10e332a157a491298195acf15a421

This fixed my problem:

$ npm ls clappr
└── clappr@0.2.63 
$ grep "Your browser does not support the playback of this video" build/app.js | wc -l
1
tjenkinson commented 7 years ago

This plugin (and others) should have its externals webpack config like this: https://github.com/clappr/clappr/issues/1145#issuecomment-246009624

We should make a note of this somewhere in the readme

tjenkinson commented 7 years ago

Also clappr-zepto should be accessed on clappr itself so doesn't need to be in the package.json or webpack external

leandromoreira commented 7 years ago

@tjenkinson do you mean?

   -library: 'LevelSelector',
   +library: ['level-selector', 'LevelSelector'],