Skip to content
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

tries to load node_modules/vtt.js/dist/vtt.js over network #2135

Closed
marcing opened this issue May 11, 2015 · 30 comments
Closed

tries to load node_modules/vtt.js/dist/vtt.js over network #2135

marcing opened this issue May 11, 2015 · 30 comments
Labels

Comments

@marcing
Copy link

marcing commented May 11, 2015

"NetworkError: 404 Not Found - http://localhost/node_modules/vtt.js/dist/vtt.js"

I think this is not meant to be ran client side.

@heff
Copy link
Member

heff commented May 11, 2015

It should only do that if there's no VTT object on the WebVTT object on the window.

if (typeof window['WebVTT'] !== 'function') {

Are you not using the main version that includes VTT.js? Or can you put up a live demo or jsbin of what you're doing?

@heff heff added the bug label May 11, 2015
@marcing
Copy link
Author

marcing commented May 11, 2015

I am using the novtt version.

@heff
Copy link
Member

heff commented May 11, 2015

Are you using tracks or not? If you are using tracks you need to include your own copy of VTT.js in the page before video.js. If you're not using tracks, then this might be considered a bug.

@marcing
Copy link
Author

marcing commented May 12, 2015

I don't think it should try to load a node module in any case in a web browser.

@mmcc
Copy link
Member

mmcc commented May 12, 2015

I'm not sure if this was meant for debugging and accidentally left in, but here's why you're seeing this. Is this intentional, @gkatsev?

@gkatsev
Copy link
Member

gkatsev commented May 12, 2015

Yes. It was intentional. It probably needs to be reworked a bit, especially to not load it if tracks aren't being used.

@mmcc
Copy link
Member

mmcc commented May 12, 2015

Ok cool, just making sure it wasn't an artifact from development.

It seems really weird to try and request something out of the node_modules directory like this. We don't include that in dist, won't put it on the CDN, and shouldn't expect a client side dev to ever have it, so it just feels...weird. What's the reasoning behind the reference?

@erkattak
Copy link

erkattak commented Aug 3, 2015

Is there a difference between the video.js and video.novtt.js builds in the 4.12.11 builds that can be downloaded from the site?

Each of those fails on trying to load from node_modules, which seems downright wrong to expect in a browser environment.

@heff
Copy link
Member

heff commented Aug 3, 2015

video.js includes vtt.js in the bundle, so it shouldn't even be trying to load the node_modules version. If it is that's a bug. If it is could you set up a jsbin showing the issue? http://jsbin.com/axedog/edit?html,js,output

@erkattak
Copy link

erkattak commented Aug 6, 2015

I think this had to do with the way I was requiring it from webpack

@heff
Copy link
Member

heff commented Aug 6, 2015

Is it something others might do too or your unique to your setup?

@cherihung
Copy link

I'm also seeing this requiring the full video.js using webpack. but only when a file needs to use the flash (video-js.swf) player. error breaks on this line as pointed out in an earlier comment.

I'm not using tracks. Is there a way to forcefully turn off tracks completely?

@erkattak
Copy link

erkattak commented Sep 6, 2015

I'm not using tracks at all and got things working by using video.novtt.js and imports-loader like so

import videojs from 'imports?this=>window!./lib/video.novtt.dev.js'

@gkatsev
Copy link
Member

gkatsev commented Nov 17, 2015

Fixed in #2741, I believe.

@gkatsev gkatsev closed this as completed Nov 17, 2015
@baseten
Copy link

baseten commented Nov 29, 2016

I'm still getting this error in the current build, compiling with webpack using import videojs from 'video.js'. I'm guessing this #3445 may fix it? What's the timescale on this getting merged into master?

If I try to import the dist instead import videojs from 'video.js/dist/video.js' I get the problems described in #2750

@Ambroos
Copy link

Ambroos commented Dec 8, 2016

@baseten Timing. I just encountered this too, with webpack.

Easy fix:

npm install --save videojs-vtt.js and npm install --save-dev file-loader.

Then where you start your VideoJS or create it's config:

import vttJsPath from 'file-loader?name=vtt.js.[hash].[ext]!videojs-vtt.js/dist/vtt.min.js';

...
const videoJSConfig = {
  'vtt.js': vttJsPath,
  ...
};

It ain't pretty but it works here on 5.12.6.

@adamwathan
Copy link

Seeing this problem as well but only in FireFox, even though I'm not using tracks.

As far as I can tell, this return statement is never hit, even when there are no tracks:

https://github.com/videojs/video.js/blob/master/src/js/tech/tech.js#L472

...because it's just checking if an object is falsey, and even though tracks.length is 0, !tracks is still always true.

Problem goes away if I use the CDN version too though, just annoying not to be able to bundle with the rest of my JS via Webpack.

@adamwathan
Copy link

Just checked the latest pre-release, this now happens in Chrome as well, not just FireFox.

@claudiorodriguez
Copy link

Also seeing this in Chrome with Webpack - not using tracks.

@BeKnowDo
Copy link

Seeing this issue as well...

@claudiorodriguez
Copy link

A simple workaround seems to be to add this to webpack config:

resolve: {
  alias: {
    'video.js': 'video.js/dist/alt/video.novtt'
  }
}

@theGC theGC mentioned this issue Dec 21, 2016
7 tasks
@theGC
Copy link

theGC commented Dec 21, 2016

If you're using webpack then the current method to add WebVTT support is broken. The simplest workaround is to add the npm module videojs-vtt.js to your window, i.e.

window.WebVTT = require('videojs-vtt.js').WebVTT

videojs-vtt.js is already a requirement of video.js however it doesn't attach it to your window unless it sees your browser needs it (although as pointed out below this fails - hence the errors).

The method at fault is: https://github.com/videojs/video.js/blob/master/src/js/tech/tech.js#L525

It is currently trying to set a scripts src to: script.src = this.options_['vtt.js'] || '../node_modules/videojs-vtt.js/dist/vtt.js'

options['vtt.js'] would be fine if you were including an external file - but I'd assume most users aren't doing this.

The alternative ../node_modules/videojs-vtt.js/dist/vtt.js string will just fail for 99.9% of use cases as you are rather unlikely to be including node_modules as part of your distribution, and even then it has to be in the right (relative) directory and accessible!

It seems to me that the module videojs-vtt.js should be loaded and polyfill the browser by default when required.

Alternatively if the user is utilising tracks then we warn them that they need to manually include videojs-vtt.js in their build process. That way if they are not you save the unwanted file bloat which I assume is the only reason for not choosing the previous option.

@gkatsev
Copy link
Member

gkatsev commented Dec 21, 2016

I was going to point out that we're fixing it (or trying to: #3794) but then I realized that this only solves it for people loading in the dist file or browserify. I'm going to make sure our solution ends up being bundler agnostic.

@ngocketit
Copy link

Thanks @theGC! That seems to solve the issue temporarily for me.

@gkatsev
Copy link
Member

gkatsev commented Jan 11, 2017

Just merged #3919, it should fix the issue for webpack and browserify users.

@natorojr
Copy link

Thanks, guys! Just curious, when is this slated for release?
I'm current on 5.15.1

@gkatsev
Copy link
Member

gkatsev commented Jan 15, 2017

It's been released as 5.16.0 which is currently next on npm. Please give it a shot. We generally wait at least a week before promoting a release to latest to make sure that we find any major bugs before the promotion.

@natorojr
Copy link

Thanks, @gkatsev! Really appreciate it.

@natorojr
Copy link

I just updated my video.js npm package to v5.16.0, re-bundled my app with Webpack, and ran a small set of video player smoke tests.

Confirming the problem is fixed for me. I'm no longer seeing the errors in the console.

THANKS, AGAIN!

@gkatsev
Copy link
Member

gkatsev commented Jan 21, 2017

@natorojr thanks for verifying!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests