Kibana: Aliasing imports with `as`

[stacey-gammon]

During a recent refactor for converting default to named exports the question came up for whether or not, or when, to alias imports with as.

e.g:

import { MyNamedExport as Export } from 'my/named/export';

vs

import { MyNamedExport } from 'my/named/export';

Below, @kobelb brought up the inverse situation:

import { Export as MyNamedExport } from 'my/named/export';

vs

import { Export } from 'my/named/export';

In that case, I suggest the export should be renamed, if we have control over it, since IMO, it's a code smell indicating an overly generic name choice.

Some background:

• Meta issue for converting default to named: https://github.com/elastic/kibana/issues/11220
• Original discussion for default to named: https://github.com/elastic/kibana/issues/8641
• PR which rose the debate: https://github.com/elastic/x-pack-kibana/pull/968

My preference is to add a style guide rule to not alias imports except in the following cases:

• there is a naming collision
• to add context to an import that is very generic, and renaming that export is either not a possibility, or would be a significant effort to do so.

My main reason is that is allows for inconsistent naming, which is one of the main benefits of enforcing named exports. The biggest benefit of consistent naming is for refactoring and code readability.

If you agree with a style guide rule to avoid the use of as unless there is very good reason (see above), 👍 this issue. If you prefer more flexibility and don't wish for this to be in the style guide, 👎 this issue. If you have another preference, comment, and if you don't care either way, then you can go about your day. 😸

cc @ycombinator @kobelb

[ycombinator]

I think the guideline here should be something like: avoid using as unless there is a namespace collision amongst imports or the imported name (without as) would be too generic or ambiguous.

[stacey-gammon]

I updated the original issue @ycombinator to take your suggestion and be more explicit about when as is acceptable.

[kobelb]

For those of you that don't wish to read through my PR that inspired the controversy, the specific situation was doing:

import { events as esqueueEvents } from './esqueue';

While I do agree with @Stacey-Gammon that we should generally be avoiding using as, I do see benefit when it's used to provide additional context, which I felt that it added to the specific situation I was using it.

[kobelb]

Just for clarification, I think that the example that @Stacey-Gammon provided is a bad use of as where it's taking something specific and making it more generic:

import { MyNamedExport as Export } from 'my/named/export';

where if it was doing the following, I would see it as a potential good use of as:

import { Export as MyNamedExport } from 'my/named/export';

[stacey-gammon]

Thanks for providing more context @kobelb.

I'd be willing to add a caveat to the rule that if we don't have control over the name of the export, and it's very generic, that it's okay to alias.

But if it's our code and an import needs to be aliased because it's too generic, then I think that points to a bad name choice for the export. In the specific situation above, you chose the name events as the export from ./esqueue/constants/events. In that case, I think the right solution would be to change the name of the export (because it's our code) to esqueueEvents, which then removes the need to alias in order to make it more specific.

[kobelb]

I see there being situations where based on where the code is used, it makes sense to refer to something by different names. Within the esqueue module, it makes sense to refer to just events because of the context; but if we wish to use this outside of esqueue the additional scoping via the as keyword provides benefit in my opinion.

I see it being similar to the general guidance for variable naming. Based on the scope of a variable, different names are more appropriate. I would never name a global variable i, but I would use it as a variable within a for loop, and they could both be pointing to the same place in memory.

[stacey-gammon]

Sounds like we have discovered the real underlying disagreement. :)

I really dislike generic names because it can be very difficult to search for usages. Searching for events in x-pack-kibana shows 436 hits. Searching for esqueueEvents shows 14 (half of which are in the coverage report). But if you allow aliasing, you don't know to search for esqueueEvents - one place might use as esqueueEvents while another uses as eventsEsqueue. Maybe instead you can search for the file name, but if the file name is also just events, that isn't helpful either. Which brings me back to my initial issue with default exports and all the various ways they were being imported in our code base.

[kobelb]

I like generic names when used in the right context because I dislike prefixing every class with the same thing, imagine if every named export in the esqueue folder was prefixed with Esqueue/esqueue. I see the esqueue module itself as providing that context, and it's up to the consumer to determine whether it should be more specific or not.

[stacey-gammon]

Yea, I see what you are saying, and I wouldn't suggest going so far as to prefix everything with Esqueue. There is a balance - take a look at some of the vislib code for the completely opposite side of the spectrum which doesn't just have a prefix but includes the entire path name in the export names. I think that is going too far.

I think the decision should be:
Opt for a more specific name when either of the following are true:

• The name of your export is a very generic name and doesn't provide any context:
  
  
  
  • e.g. Constants, Events, Strings.
    
    
    
    
    
    • The export is used externally
    
    
    
    • e.g. the esqueue/constants/events export is used outside of the esqueue folder, so we know in that situation the extra context will be helpful.
    
    
    
    • When we start using index.js there will be an ever clearer line between which exports are available externally and which are mean to be kept internal.
    
    
    
  
  

[w33ble]

I think the guideline here should be something like: avoid using as unless there is a namespace collision amongst imports

Agreed

or the imported name (without as) would be too generic or ambiguous.

That's very subjective. Clearer rules would be good if we're going to try to enforce something.

I would also encourage explicit rules for using as with *, ala import * as SomeModule from './path/to/some/module.

[stacey-gammon]

@w33ble - you thumbs downed the issue but your comment says agreed on it's main point. Which is the part on the main issue that you disagree with?

I'm trying to think of a way to improve consensus since we are nearly split.

@tsullivan, @jbudz - if I added a caveat that using import alias' is also acceptable if it's to provide additional context, and you aren't able rename the export (e.g. it's not our code, or too significant of an effort), would it change either of your minds?

@w33ble - good point regarding *. It's interesting because that style was suggested by @kjbekkelund as a way to namespace exports instead of using a class with static functions, mentioned in this issue: https://github.com/elastic/kibana/issues/10844.

But as soon as you use as, it creates the opportunity for naming inconsistencies, so I'm inclined to also limit it.

[ycombinator]

I would also encourage explicit rules for using as with *, ala import * as SomeModule from './path/to/some/module.

My personal preference is to avoid the * and use explicit names instead. I realize this becomes an issue when there are a lot of exports being imported but a) that's usually rare and b) it might point to a smell with the exporting module doing too much (e.g. a utils module could be broken into string_utils and number_utils). Thoughts?

[kimjoar]

I almost never rely on *, but it definitely has valid use-cases where they aren't a code smell.

[ycombinator]

@kjbekkelund said:

I almost never rely on *, but it definitely has valid use-cases where they aren't a code smell.

Could you give an example (or, even better, a pattern)? Might be good to note it in the style guide as an exception.

[kimjoar]

e.g. I could easily see myself doing it in tests, where the code is exported as mentioned in https://github.com/elastic/kibana/issues/10844

[kimjoar]

But again: rarely. But I just prefer having the option instead of "nothing is allowed"

[kimjoar]

For Cloud UI we default export the reducer, then export function x for all our helpers. Then in reducers/index.js we do:

import auth, * as fromAuth from './auth'

We can then:

export default combineReducers({
  auth
});

// helper
export const getCurrentUser = state =>
  fromAuth.getUser(state.auth)

which has been a very nice pattern for our reducers.

(Very specific to our Redux setup, though. Just an example where "strict rules" might not make sense)

[kimjoar]

Example with some explanation by Dan Abramov: https://egghead.io/lessons/javascript-redux-colocating-selectors-with-reducers

[kimjoar]

I'm okey with having something along the lines of "you usually shouldn't" in the styleguide, but going to the length of saying "not allowed" or having an eslint fail the build would be a 👎 for me.

Later on we can add examples etc. But right now I definitely see potential use-cases for as.

[w33ble]

Which is the part on the main issue that you disagree with?
I'm trying to think of a way to improve consensus since we are nearly split.

OK, I changed back to no vote. It sounds like there's still some vagueness for what people would like to see as an eslint rule. I'd just like to see some consensus around the rules before strict enforcement is added.

[stacey-gammon]

Thanks for the feedback @w33ble and @kjbekkelund - I updated the initial comment to take out mention of any new eslint rules and modified the rule to be:

My preference is to add a style guide rule to not alias imports except in the following cases:

• there is a naming collision
• to add context to an import that is very generic, and renaming that export is either not a possibility, or would be a significant effort to do so.

[kobelb]

Even without this being implemented as an eslint rule, I still find the language to be rather restrictive and would prefer we give ourselves some leeway on when to use "as", so I will be leaving my vote as a "No".

[Bargs]

I changed from a yes to a no after reading @kobelb's example. I'd be ok with a guideline that says something along of lines of "don't use as unless you have a good reason". Using the same variable name everywhere is nice, but ultimately it's not a huge benefit, static analysis makes it just as easy to find all the usages of a given export. And in @kobelb's example as definitely improves readability.

[w33ble]

to add context to an import that is very generic

Wouldn't that recommendation cover usage for readability? It sounds like this wouldn't be a hard rule, but more of a suggestion. Kind of like our "prefer native over lodash" rule.

[stacey-gammon]

I'd be ok with a guideline that says something along of lines of "don't use as unless you have a good reason".

@Bargs, Can you be more specific about what you think should qualify as a good reason, or why you disagree with the reasons I gave?

I'm not sure which specific example you are referring to since there were quite a few, but if it was the one from the PR - the events as esqueueEvents - then my contention was that the export should just be named esqueueEvents, instead of events, to negate the need for the alias.

If everyone agrees on language that provides a bit more leeway, I can adjust it to be without good reason, but we are going to end up with arguments about what qualifies as a "good reason" (see the PR for an example). I was trying to be explicit to avoid those arguments.

I just manually switched over about 1,000 files from default to named, so I feel pretty strongly about not degrading the reason behind the effort. Switching to named and using as everywhere completely overrides the reason for named exports.

Also re: static analysis -- @Bargs I think you are going to have to help me with my IDE set up because if I tell intellij to "find usages" of the default exportrootNotifier in ui/notify/notify.js it comes up with 3 that are in that file, when really it's referenced ~20-30 times in other files, including some in x-pack. Does your IDE tell you that?

[kobelb]

I've been thinking more about this situation, and I'm not sure that using as is vastly different than the following scenario:

import { somethingNamedRight } from './somethingNamedRight';

const somethingNamedRightEvents = somethingNamedRight.events;

The above is something that I wouldn't normally find offensive in any manner. This isn't being done using an as as it applies to a field on the named export, but it seems like an eerily similar situation to me.

[Bargs]

The stated benefits of using named exports are easier refactoring and improved readability.

As @kobelb showed in the comment above me there are other ways to frustrate the named export rule besides using as. And if we have any exceptions to the rule (e.g. for conflicts, or bad names in third party libs) searching by name will never be a bulletproof way to find all uses of an export. So the improved refactoring argument isn't compelling to me.

That only leaves readability, which I think is a good benefit. But we've already identified a couple situations where as helps readability more than it hurts (conflicts or bad names in third party libs). I'm just not certain we've thought of every situation where it might help.

Switching to named and using as everywhere completely overrides the reason for named exports.

I don't think we'll start using as everywhere, I think it'll be pretty rare. I almost never use as myself. In the common case, named exports will still provide a lot of benefit. If as becomes a widespread problem we could always reconsider.

if I tell intellij to "find usages" of the default exportrootNotifier in ui/notify/notify.js

It's because that import path is using a webpack alias which we need to get rid of.

[w33ble]

I don't think we'll start using as everywhere, I think it'll be pretty rare. I almost never use as myself.

I used to be in the same boat, but this becomes an issue with React/Redux, if we're going to follow the component/container paradigm and the "everything is a named export" rule. Here's an example container, which wraps a component and passes state as props via the connect helper from react-redux:

// containers/MyComponent.js
import { connect } from 'react-redux';
import { MyComponent as Component } from '../components/MyComponent';

const mapStateToProps = (state) => {
  return {
    prop: state.prop,
  };
};

export const MyComponent = connect(
  mapStateToProps,
)(Component);

If I want to name both of these things the same, I can't import the component as MyComponent, because I'm required to export the container as a named thing, MyComponent, and the MyComponent defintion can't exist twice or the linter will error.

So unless we decide that we want to be explicit about container naming and somehow name them differently, as is going to be a common escape hatch.

All of my containers look like this, btw. It's also quite nice because it makes the react-redux boilerplate simple; I just copy an existing container, replace all occurrences of MyComponent with whatever the name of the component I want to wrap is, and change the object in mapStateToProps. Easy peasy.

This isn't a for or against comment, just pointing out a pattern that may become commonplace.

[stacey-gammon]

Final decision is: our general suggestion is to use import as aliasing sparingly but this is ultimately a decision left to coder and reviewers - no hard and fast rule.