Decide on a final var style

[heff]

See #1993 (diff)

Now that we are using let in the codebase we can define variables where they're used, but there's still some open style questions that we should lock down.

---

TODO

• Update code to match videojs-standard
• Merge in videojs-standard checking
• Document videojs-standard and usage of the airbnb style guide.

[dmlap]

I think AirBnB's es6 style guide is really comprehensive and I don't hate a single thing in it (shocking!). Thoughts on adopting it wholesale for 5.0?

[heff]

Yeah, I like the idea. It's grown a bit since the last time I looked at it so I need to review again, but I didn't see anything I couldn't live with. I think it's interesting they default to const for variables. This issue has a few more details.

• Why? This ensures that you can't reassign your references (mutation), which can lead to bugs and difficult to comprehend code.
• If you must mutate references, use let instead of var.

I don't have any objections to that myself.

Whether it's AirBnB or another style guide, I love the idea of adopting an external public style guide, with zero exceptions. We could save a bunch of back and forth by shifting all responsibility and discussions around style to the external guide, not our projects.

[gkatsev]

I generally like their style guide, though, some of the reasoning and terminology is weird.
There are two things I don't agree with, though. The first strongly, the second moderately.

preferring function declarations over function expressions for their naming and hoisting

You can have named function expressions and relying on hoisting is bad.

Not to use wildcard imports

I think this makes sense for modules that export an object or something concrete but for the "bag of functions" modules I think it's not as good. If no default export you can just do import {foo, bar} from "./lib.js"; to just get what you need instead of just doing import * as Lib from "./lib.js"; or import Lib from "./lib.js";

[heff]

relying on hoisting is bad

I can't find any discussions around this in the guide repo. Want to submit an issue and see what they respond with?

but for the "bag of functions" modules I think it's not as good

It doesn't look like they're saying not to use import {foo, bar} from "./lib.js";, just literally the wildcard import * as .... The example just above this note has import { es6 } from './AirbnbStyleGuide';.

[gkatsev]

If they also talk about not using import * that's fine.

As for hoisting and functions, they mentioned liking function statements over function expressions in this section. They also talk about hoisting in general here but as pointed in issue airbnb/javascript#273 they don't have a concrete rule about hoisting other than in the functions section. I agree with the OP of the issue that relying on hoisting is bad for similar reasons that they mention in that issue.

[heff]

@gkatsev does that mean you care enough about the function hoisting to not adopt the guide wholesale?

The main complaint in the function hosting issue is the linear readability of the code, and I don't actually share that opinion myself. I like the organization options of hoisted functions.

[gkatsev]

I'm OK with adopting it except for the preference for function declarations over function expressions with hoisting. Relying on hoisting is bad and could lead to issues. This is one of the reasons why previously a lot of style guides and linters required variables to be defined at the top of the scope. Preferring function declarations over function expressions can lead one to rely on hoisting.

Also, seems like they're actively working on changing this style with feedback from the community. This is both good and bad. It's good because it'll get more rules ironed out, like the question about hoisting. It's bad because it won't necessarily align with our opinions. Something to consider.

Perhaps a solution is to snapshot their styleguide for our own purposes (via a fork) and add in what we want and then send PRs if we feel like some specific items are worth sharing with the community at large.

[gkatsev]

For hack week, I had another idea for a quick project that could be done. Fork feross/standard into videojs/standard and modify it to use the rules that we want and care about and then update all our projects to use that. That way we won't need to figure out how to configure each individual tool for new projects and all our projects will have the exact same tooling around style and linting.

[mmcc]

If we're going to officially start saying "this is our style guide, adhere to it," I think we have to go with a fork. I think we should avoid actively diverging from the original, but that allows us to decide on updates and update our code base to match.

Huge +1 on the feross/standard fork. If someone tries to take away semicolons I'm going to burn everything to the ground, but otherwise that sounds cool.

[heff]

To me there's a huge advantage to not forking. It means it would no longer be our responsibility to have internal discussions around style, which are a time drain and don't make a huge difference when you get down to the minutia. I'd be willing to sacrifice a good amount of preferences to get there. It doesn't mean we can't change anything, it just means we have to work with a larger community to do so, assuming the guide is open to community input which it seems to be.

[mmcc]

I follow the logic, but I think if we do this we should not change anything to our preference on the fork. The fork would just give us time to adjust to any potential changes if they shift the style guide. Frankly, I don't feel too strongly about this, so I'm fine with either path. I think it's fairly safe to assume the guide won't change too drastically, but it would be awkward if the guide changed without us noticing and we were reviewing PRs on outdated information.

[gkatsev]

By forking I meant more like the github fork button. As @mmcc mentioned, it means that we can apply changes from upstream as we want, kind of like a dependency from npm where we can update when we want.

Ultimately, the tools we use is what actually dictates our styles. So, codifying the current guideline will be more helpful than any other thing we do.

[heff]

Got it. So you're saying we'd fork it just to lock it down, not to add our own modifications.

[mmcc]

Precisely.

[dmlap]

Conclusion: Fork AirBnB guide and adopt.

• Update some guide somewhere that says we did this
• Update linter to match the guide

Anything else relevant.

[gkatsev]

I'll take this one.

[dconnolly]

Citations to update when this is ready:

• CONTRIBUTING.md

[heff]

@gkatsev can we keep moving forward with this one? What's the next step here, convert another big file?

[gkatsev]

I think now that the Tech 2.0 stuff has been merged, we could either do a massive style PR to go along with #2099.

[heff]

Ok sounds good. Will you have time to work on that any time soon? I'm
working on #2010 (breaking up Lib) and that should either come before or
after this one since it also touches a lot of files.

On Mon, May 11, 2015 at 10:32 AM, Gary Katsevman [email protected]
wrote:

I think now that the Tech 2.0 stuff has been merged, we could either do a
massive style PR to go along with #2099
#2099.

—
Reply to this email directly or view it on GitHub
#2034 (comment).

[heff]

Seeing as we're trying to get the first release candidate out on Friday this week, I don't think we're going to finish this up by then. Moving it to the 5.1 milestone since it's not API breaking. If there are any related API changes that I'm missing we should get just those in ASAP.

[gkatsev]

We've decided on the style, so, going to close this. We still need to actually set up standard and lint to follow this style but those are separate.