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

videojs-standard #3459

Closed
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@misteroneill
Member

misteroneill commented Jul 25, 2016

Description

As discussed in-person on Thursday, we are in a slight lull at the moment and this is a great opportunity to clean up the video.js source code (and tests) to pass videojs-standard linting and abandon jshint.

Specific Changes proposed

  • Removes jshint
  • Installs videojs-standard at 5.x
  • Adds linting scripts/tasks
  • Makes small code changes to address videojs-standard checks against src/js/*.js

Requirements Checklist

  • Feature implemented / Bug fixed
  • Reviewed by Two Core Contributors
@@ -114,14 +114,6 @@ module.exports = function(grunt) {
build: ['build/temp/*'],
dist: ['dist/*']
},
jshint: {

This comment has been minimized.

@misteroneill

misteroneill Jul 25, 2016

Member

This is a temporary step. The final PR in this series will restore automatic linting capabilities, but for now we want them off.

@misteroneill

misteroneill Jul 25, 2016

Member

This is a temporary step. The final PR in this series will restore automatic linting capabilities, but for now we want them off.

@@ -3,10 +3,7 @@
*/
import ClickableComponent from './clickable-component.js';
import Component from './component';
import * as Events from './utils/events.js';

This comment has been minimized.

@misteroneill

misteroneill Jul 25, 2016

Member

There are removals like this scattered around: they were unused modules.

Note: if we're only trying to import the module, we should be doing something like import './utils/events'.

@misteroneill

misteroneill Jul 25, 2016

Member

There are removals like this scattered around: they were unused modules.

Note: if we're only trying to import the module, we should be doing something like import './utils/events'.

This comment has been minimized.

@gkatsev

gkatsev Jul 25, 2016

Member

doesn't import './utils/events.js' put the exports into local scope? This is an issue for a "bag of functions" module.
import * Events from './utils/events.js' is more explicit in what we want.

@gkatsev

gkatsev Jul 25, 2016

Member

doesn't import './utils/events.js' put the exports into local scope? This is an issue for a "bag of functions" module.
import * Events from './utils/events.js' is more explicit in what we want.

This comment has been minimized.

@misteroneill

misteroneill Jul 25, 2016

Member

I tend to agree, but the linter doesn't like unused variable names.

@misteroneill

misteroneill Jul 25, 2016

Member

I tend to agree, but the linter doesn't like unused variable names.

This comment has been minimized.

@gkatsev

gkatsev Jul 25, 2016

Member

if it isn't used then we shouldn't import it, sure.

@gkatsev

gkatsev Jul 25, 2016

Member

if it isn't used then we shouldn't import it, sure.

This comment has been minimized.

@misteroneill

misteroneill Jul 25, 2016

Member

On the topic of using the import 'foo'; style, I don't think it puts the exports into local scope. I think it simply runs the module code. MDN says:

Import an entire module for side effects only, without importing any bindings.

@misteroneill

misteroneill Jul 25, 2016

Member

On the topic of using the import 'foo'; style, I don't think it puts the exports into local scope. I think it simply runs the module code. MDN says:

Import an entire module for side effects only, without importing any bindings.

This comment has been minimized.

@misteroneill

misteroneill Jul 25, 2016

Member

Same as require('foo.js');

@misteroneill

misteroneill Jul 25, 2016

Member

Same as require('foo.js');

This comment has been minimized.

@gkatsev

gkatsev Jul 25, 2016

Member

Ah, interesting. I guess that's exactly what we want in player.js.

@gkatsev

gkatsev Jul 25, 2016

Member

Ah, interesting. I guess that's exactly what we want in player.js.

Show outdated Hide outdated src/js/clickable-component.js
addChild(child, options={}) {
// TODO: Fix adding an actionable child to a ClickableComponent; currently
addChild(child, options = {}) {
// TO_DO: Fix adding an actionable child to a ClickableComponent; currently

This comment has been minimized.

@misteroneill

misteroneill Jul 25, 2016

Member

videojs-standard doesn't want you leaving TODO comments around (which makes sense because they almost never get done and only cause confusion down the line), but I didn't want to simply delete them. So, I changed them to TO_DO which is kind of ugly, I know.

@misteroneill

misteroneill Jul 25, 2016

Member

videojs-standard doesn't want you leaving TODO comments around (which makes sense because they almost never get done and only cause confusion down the line), but I didn't want to simply delete them. So, I changed them to TO_DO which is kind of ugly, I know.

This comment has been minimized.

@BrandonOCasey

BrandonOCasey Jul 25, 2016

Contributor

I think the TODO rule is just a warning, should we just keep the warnings printing and work on fixing whatever it is we need to do if we want to get rid of them?

@BrandonOCasey

BrandonOCasey Jul 25, 2016

Contributor

I think the TODO rule is just a warning, should we just keep the warnings printing and work on fixing whatever it is we need to do if we want to get rid of them?

This comment has been minimized.

@misteroneill

misteroneill Jul 25, 2016

Member

You're right, I'll undo these changes!

@misteroneill

misteroneill Jul 25, 2016

Member

You're right, I'll undo these changes!

const styles = '{{GENERATED_STYLES}}';
if (styles === '{{GENERATED'+'_STYLES}}');
// Don't think we need this as it's a noop?
// if (styles === '{{GENERATED'+'_STYLES}}');

This comment has been minimized.

@gkatsev

gkatsev Jul 25, 2016

Member

I assume it was supposed to be something like

if (styles === '{{GENERATED'+'_STYLES'}}) {
  return;
}

So that if somehow the styles weren't inline, it won't bother with the rest.

@gkatsev

gkatsev Jul 25, 2016

Member

I assume it was supposed to be something like

if (styles === '{{GENERATED'+'_STYLES'}}) {
  return;
}

So that if somehow the styles weren't inline, it won't bother with the rest.

This comment has been minimized.

@misteroneill

misteroneill Jul 25, 2016

Member

Hmm, thoughts on what to do, since it's clearly never done that?

@misteroneill

misteroneill Jul 25, 2016

Member

Hmm, thoughts on what to do, since it's clearly never done that?

This comment has been minimized.

@gkatsev

gkatsev Jul 25, 2016

Member

base-styles isn't even used anywhere anyway right now, so, feel free to just leave as is.

@gkatsev

gkatsev Jul 25, 2016

Member

base-styles isn't even used anywhere anyway right now, so, feel free to just leave as is.

This comment has been minimized.

@misteroneill
@misteroneill
Show outdated Hide outdated src/js/player.js
import ControlBar from './control-bar/control-bar.js';
import ErrorDisplay from './error-display.js';
import TextTrackSettings from './tracks/text-track-settings.js';
import './tech/loader.js';

This comment has been minimized.

@gkatsev

gkatsev Jul 25, 2016

Member

I would much prefer the earlier version. It's much more explicit and reduces cognitive load. I don't have to go to ./tracks/text-track-settings.js to know that in here I can refer to it via TextTrackSettings.

@gkatsev

gkatsev Jul 25, 2016

Member

I would much prefer the earlier version. It's much more explicit and reduces cognitive load. I don't have to go to ./tracks/text-track-settings.js to know that in here I can refer to it via TextTrackSettings.

This comment has been minimized.

@misteroneill

misteroneill Jul 25, 2016

Member

We'd need to turn off the linter for that line, then.

@misteroneill

misteroneill Jul 25, 2016

Member

We'd need to turn off the linter for that line, then.

This comment has been minimized.

@gkatsev

gkatsev Jul 25, 2016

Member

What rule complains about this?

@gkatsev

gkatsev Jul 25, 2016

Member

What rule complains about this?

This comment has been minimized.

This comment has been minimized.

@gkatsev

gkatsev Jul 25, 2016

Member

oooh, these are imported to make sure they're in the package. They're not used locally in player.js.
I wonder if there's a better way of handling this.
I guess this is fine then

@gkatsev

gkatsev Jul 25, 2016

Member

oooh, these are imported to make sure they're in the package. They're not used locally in player.js.
I wonder if there's a better way of handling this.
I guess this is fine then

This comment has been minimized.

@misteroneill

misteroneill Jul 25, 2016

Member

Could also do require('./tech/loader.js'). I suppose it might be a little clearer that it's different from other imports?

@misteroneill

misteroneill Jul 25, 2016

Member

Could also do require('./tech/loader.js'). I suppose it might be a little clearer that it's different from other imports?

This comment has been minimized.

@gkatsev

gkatsev Jul 25, 2016

Member

We do have a comment above. Maybe just elaborate on it a bit?

@gkatsev

gkatsev Jul 25, 2016

Member

We do have a comment above. Maybe just elaborate on it a bit?

Show outdated Hide outdated src/js/player.js
if (/^[^a-zA-Z]/.test(this.id())) {
idClass = 'dimensions-'+this.id();
if ((/^[^a-zA-Z]/).test(this.id())) {
idClass = `dimensions-${this.id()}`;

This comment has been minimized.

@gkatsev

gkatsev Jul 25, 2016

Member

did the linter complain about this? The reason we kept it the other way is because using the template strings here gives us no benefits over just string concatenation, and if anything, the old way is a bit cleaner.

@gkatsev

gkatsev Jul 25, 2016

Member

did the linter complain about this? The reason we kept it the other way is because using the template strings here gives us no benefits over just string concatenation, and if anything, the old way is a bit cleaner.

This comment has been minimized.

@misteroneill

misteroneill Jul 25, 2016

Member

It complained about the infix operator not having spaces; so, I figured I'd just switch to the template string. Can switch back if desired.

@misteroneill

misteroneill Jul 25, 2016

Member

It complained about the infix operator not having spaces; so, I figured I'd just switch to the template string. Can switch back if desired.

This comment has been minimized.

@gkatsev

gkatsev Jul 25, 2016

Member

I think that in this particular case it would be a bit nicer.

'dimensions-' + this.id();
`dimensions-${this.id()}`;
@gkatsev

gkatsev Jul 25, 2016

Member

I think that in this particular case it would be a bit nicer.

'dimensions-' + this.id();
`dimensions-${this.id()}`;

This comment has been minimized.

@misteroneill

misteroneill Jul 25, 2016

Member

Fair enough! 👍

@misteroneill

misteroneill Jul 25, 2016

Member

Fair enough! 👍

@@ -2666,8 +2712,10 @@ class Player extends Component {
*/
// destructure the input into an object with a track argument, defaulting to arguments[0]
// default the whole argument to an empty object if nothing was passed in
removeRemoteTextTrack({track = arguments[0]} = {}) { // jshint ignore:line
this.tech_ && this.tech_['removeRemoteTextTrack'](track);

This comment has been minimized.

@gkatsev

gkatsev Jul 25, 2016

Member

my one liner T__T

@gkatsev

gkatsev Jul 25, 2016

Member

my one liner T__T

This comment has been minimized.

@misteroneill

misteroneill Jul 25, 2016

Member

🔥 burn it down 🔥

@misteroneill

misteroneill Jul 25, 2016

Member

🔥 burn it down 🔥

Show outdated Hide outdated src/js/player.js
/**
* Fired when the user agent begins looking for media data
*
* @event loadstart
*/
Player.prototype.handleTechLoadStart_;
Player.prototype.handleTechLoadStart_ = Player.prototype.handleTechLoadStart_;

This comment has been minimized.

@gkatsev

gkatsev Jul 25, 2016

Member

Does the linter complain about these? I think it would be nice to keep them as stubs, if possible.

@gkatsev

gkatsev Jul 25, 2016

Member

Does the linter complain about these? I think it would be nice to keep them as stubs, if possible.

This comment has been minimized.

@misteroneill

misteroneill Jul 25, 2016

Member

Yeah. It complained about not expecting an expression. We could alternatively do something like:

Player.prototype.handleTechLoadStart_; // eslint-disable-line
@misteroneill

misteroneill Jul 25, 2016

Member

Yeah. It complained about not expecting an expression. We could alternatively do something like:

Player.prototype.handleTechLoadStart_; // eslint-disable-line
@@ -721,12 +724,12 @@ videojs.insertContent = Dom.insertContent;
* still support requirejs and browserify. This also needs to be closure
* compiler compatible, so string keys are used.
*/
if (typeof define === 'function' && define['amd']) {
define('videojs', [], function(){ return videojs; });
if (typeof define === 'function' && define.amd) {

This comment has been minimized.

@gkatsev

gkatsev Jul 25, 2016

Member

this probably should just be deleted altogether. But maybe in a separate PR.

@gkatsev

gkatsev Jul 25, 2016

Member

this probably should just be deleted altogether. But maybe in a separate PR.

This comment has been minimized.

@misteroneill

misteroneill Jul 25, 2016

Member

Yeah, I think so. Avoiding functional changes here is a good thing! 😄

@misteroneill

misteroneill Jul 25, 2016

Member

Yeah, I think so. Avoiding functional changes here is a good thing! 😄

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Jul 25, 2016

Member

LGTM

Member

gkatsev commented Jul 25, 2016

LGTM

@gkatsev gkatsev added this to the Linting milestone Jul 25, 2016

@misteroneill misteroneill referenced this pull request Jul 25, 2016

Closed

src/js/utils passing videojs-standard #3460

1 of 2 tasks complete
Show outdated Hide outdated src/js/media-error.js
@@ -8,13 +8,13 @@ import assign from 'object.assign';
*
* @param {Number} code The media error code
*/
let MediaError = function(code){
let MediaError = function(code) {

This comment has been minimized.

@gkatsev

gkatsev Jul 26, 2016

Member

noting it here (but we shouldn't make this change as part of linting) but this potentially can cause a problem on IE8 since on line 22 we do MediaError.defaultMessages and IE8 is weird.

@gkatsev

gkatsev Jul 26, 2016

Member

noting it here (but we shouldn't make this change as part of linting) but this potentially can cause a problem on IE8 since on line 22 we do MediaError.defaultMessages and IE8 is weird.

This comment has been minimized.

@misteroneill

misteroneill Jul 26, 2016

Member

Good catch. Switching to function MediaError(code) { would solve that, right?

@misteroneill

misteroneill Jul 26, 2016

Member

Good catch. Switching to function MediaError(code) { would solve that, right?

This comment has been minimized.

@gkatsev

gkatsev Jul 26, 2016

Member

Yup.

@gkatsev

@gkatsev gkatsev added minor and removed patch confirmed labels Jul 26, 2016

@misteroneill

This comment has been minimized.

Show comment
Hide comment
@misteroneill

misteroneill Jul 28, 2016

Member

I merged all the disparate vjsstandard-* branches into this one.

Member

misteroneill commented Jul 28, 2016

I merged all the disparate vjsstandard-* branches into this one.

@misteroneill misteroneill changed the title from videojs-standard Core to videojs-standard Source Code Jul 28, 2016

@misteroneill misteroneill changed the title from videojs-standard Source Code to videojs-standard Aug 1, 2016

@misteroneill

This comment has been minimized.

Show comment
Hide comment
@misteroneill

misteroneill Aug 1, 2016

Member

We are treating this as the "master" branch for standardization efforts.

Member

misteroneill commented Aug 1, 2016

We are treating this as the "master" branch for standardization efforts.

@gkatsev gkatsev closed this in 4f6cb03 Aug 5, 2016

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Aug 5, 2016

Member

Landed!

Member

gkatsev commented Aug 5, 2016

Landed!

@misteroneill misteroneill deleted the misteroneill:vjsstandard-core branch Aug 12, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment