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

AirBnb style guide deviation suggestion - use named exports #8641

Closed
stacey-gammon opened this issue Oct 12, 2016 · 14 comments
Closed

AirBnb style guide deviation suggestion - use named exports #8641

stacey-gammon opened this issue Oct 12, 2016 · 14 comments
Assignees
Labels

Comments

@stacey-gammon
Copy link
Contributor

As discussed in a prior meeting, we are planning to migrate to the airbnb style guide, but if we have reason to deviate from specific rules, we can discuss as a team. It was mentioned that preference isn't good enough, so I'm going to throw a rule out there I'd like to deviate from and my reasons.

I don't like default exports and, if it were up to me, would require named exports.

Objective reasons:

  • Makes searching for references easier
  • Helps with following and understanding the code
  • Reduces bugs when refactoring
  • Makes it easier to safely find and remove dead code.
@Bargs
Copy link
Contributor

Bargs commented Oct 13, 2016

If we get rid of our webpack aliases (which we intend to do) all of our imports should be statically analyzable so refactoring tools should allow us to find all usages of a module even if we're using default exports.

@stacey-gammon
Copy link
Contributor Author

Ah, that would definitely address all except the second bullet point. I still like named exports because it enforces a consistent naming scheme, which makes reading & understanding code easier. Though, readability may not be a strong enough reason to deviate from the style guide.

I'd like to throw it out there on Mend-it Monday, even if just for curiosity's sake, to hear some more opinions. :)

@stacey-gammon
Copy link
Contributor Author

Discussed in mend-it Monday and approved! We should now prefer named exports over default exports. Woot!

@stacey-gammon stacey-gammon changed the title AirBnb style guide deviation suggestion - default exports AirBnb style guide deviation suggestion - use named exports Oct 17, 2016
@w33ble
Copy link
Contributor

w33ble commented Nov 17, 2016

I'm reopening this to get a little clarity about using export default. I'm sorry if I missed the original discussion.

First, I understand that the following is bad:

function foo() {}
function bar() {}
export default { foo, bar };

Here, I agree that named exports make a lot more sense. This module does multiple things, and I may not need all of the methods in that module. By using named exports, we have the ability to do tree shaking and automatic dead code removal, which is awesome. And I don't think there's any question about this case.

However, I'm curious about modules that only do and export a single thing. What's correct here, named exports or default exports? It sounds like the consensus was that, contrary to the airbnb styleguide, we prefer to always use named exports.

This means that for small modules that do a single thing, instead importing them like this:

import createDocumentJobFactory from '../lib/create_document_job';
import licensePreFactory from '../lib/license_pre_routing';
import userPreRoutingFactory from '../lib/user_pre_routing';
import jobResponseHandlerFactory from '../lib/job_response_handler';

I now have to import them like this:

import { createDocumentJobFactory } from '../lib/create_document_job';
import { licensePreFactory } from '../lib/license_pre_routing';
import { userPreRoutingFactory } from '../lib/user_pre_routing';
import { jobResponseHandlerFactory } from '../lib/job_response_handler';

It seems like kind of a trivial change, but it now requires that I know how every module names its exports. I see why this might actually sound good, because it's immediately clear when a module exports a named thing that deviates from what we expect, but it feels really heavy-handed.

cc: @Bargs since he commented previously here, and @cjcenizal @spalger since they're also involved in the larger Angular styleguide discussion.

@w33ble w33ble reopened this Nov 17, 2016
@w33ble w33ble added the discuss label Nov 17, 2016
@cjcenizal
Copy link
Contributor

I agree with you Joe, but I also think it's more important we just pick a standard and stick with it. I'm fine with never ever using default exports if that's what's in our style guide.

@Bargs
Copy link
Contributor

Bargs commented Nov 17, 2016

I think I've come around on this one. It's nice to enforce consistent
naming of a function everywhere that function is imported. Another benefit
is that the module can evolve to multiple exports without any change to the
calling code. Once you've made the decision to have a default export,
you're stuck with it.

A quality editor will also autocomplete the import names, so you shouldn't
have to memorize them :)

On Thu, Nov 17, 2016, 1:47 PM CJ Cenizal notifications@github.com wrote:

I agree with you Joe, but I also think it's more important we just pick a
standard and stick with it. I'm fine with never ever using default exports
if that's what's in our style guide.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#8641 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF8zyKiYdgGP1sS-4_y4EL9f4fUqG10Zks5q_KE8gaJpZM4KVF2I
.

@w33ble
Copy link
Contributor

w33ble commented Nov 17, 2016

I also think it's more important we just pick a standard and stick with it

That's why I reopened this. I wanted to make sure we're all on board with only using named exports, mostly because the resolution on this didn't provide any details about the conversation and I think I missed it. If it was already established that we're using only named exports, so be it.

I think I've come around on this one

Meaning you're siding with named exports only? That's how I read your reply.

@w33ble
Copy link
Contributor

w33ble commented Nov 17, 2016

Maybe it's best to just ask @stacey-gammon to summarize what happened in the discussion about this. If the discussion really was just everyone agree that we should never use export default, then I can close this issue again, and we can get the styleguide updated.

@spalger
Copy link
Contributor

spalger commented Nov 17, 2016

It seems like kind of a trivial change, but it now requires that I know how every module names its exports.

This is exactly why I've been using named exports whenever I can for the last several months. I haven't regretted once.

@spalger
Copy link
Contributor

spalger commented Nov 17, 2016

Another nice thing about named imports/exports is the eslint import plugin support. It will verify your named exports/imports so if you mistype the import, or change the name of that import in the source module, eslint can help you find the code you need to update.

@w33ble
Copy link
Contributor

w33ble commented Nov 17, 2016

I'm going to close this issue again then, seems people are pretty clearly on board. Thanks!

Also, I guess we should both update the styleguide as well as explore enabling that eslint rule then. I don't suppose anyone's bother to look at how much existing code will need to be updated yet. I suspect it's a lot.

@w33ble w33ble closed this as completed Nov 17, 2016
@uboness
Copy link

uboness commented Nov 18, 2016

Named exports FTW! Thank you!

@spalger
Copy link
Contributor

spalger commented Dec 10, 2016

Reopening so that we can get this added to the styleguide

@spalger spalger reopened this Dec 10, 2016
@spalger spalger added dev and removed discuss labels Dec 10, 2016
stacey-gammon added a commit that referenced this issue Jan 12, 2017
Add new 'named exports' only rule: #8641
@stacey-gammon
Copy link
Contributor Author

Style guide updated. https://github.com/elastic/kibana/blob/master/style_guides/js_style_guide.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants