toddmotto / angularjs-styleguide

AngularJS styleguide for teams
https://ultimateangular.com
5.96k stars 700 forks source link

Named exports vs. defaults #62

Closed nomaed closed 8 years ago

nomaed commented 8 years ago

This is a great guide!

I've been working with Angular 1.5 and ES6 for several months now, trying to find good ways to make it work and make it clean. I wasn't following any guides, and I got to almost the same practices as what you describe here. Therefore, I like it :)

I'm still reading this guide, but one thing that I do a little differently is exports and imports:

I use the same export pattern with the modules themselves, using export default angular.module('appStuff', ...).name, and then in the parent directory, I import it using import appStuffModule from ... to include it in the dependencies of the "container" module.

However, I export named components explicitly using their names, and not by default export. By this I mean components (or directives, if it can't be avoided), services, factories, providers, constants, functions and basically anything that's not a module that is exported by a variable. An actual example from a project I'm working on (removed implementation and I'm also using TypeScript, but the idea is the same):

sysMonitor.ts

/**
 * @ngdoc controller
 * @name SysMonitorController
 */
class SysMonitorController {
  static $inject = ['$scope', 'dataProviderReportType', 'avrChartDataService', 'BIP_MultiDataProvider'];

  /*- Bindings -------------------------------------------------------------*/
  public timerange;

  /*- Template vars ($ctrl props) -------------------------------------*/
  public isLoading = false;
  public isOpen = true;
  // ...

  /*- Private props --------------------------------------------------------*/
  private chartConfig = <ILineChartConfig>{};
  private dataProvider: statsDataProvider;
  // ...

  /*- Public methods ----------------------------------------------------------*/
  /**
   * Constructor used for DIs and binding of methods
   */
  constructor(private $scope: ng.IScope,
              private dataProviderReportType,
              private avrChartDataService,
              private BIP_MultiDataProvider) {
    // binding callbacks from prototype to current instance
    this.onDataUpdated = this.onDataUpdated.bind(this);
  }

  public $onInit() {
    // After everything is initialized, prepare data providers and charts configuration
  }

  public $onChanges(changes) {
    // changes monitoring
  }

  public $onDestroy() {
    // remove callbacks from data providers
  }

  /*------------------------------------------------------------------------*/

  private onDataUpdated(data, error, notification) {
    // ...
  }

}

/**
 * @ngdoc component
 * @name avrSystemMonitor
 * @restrict E
 * @param timerange
 */
export const sysMonitorComponent: ng.IComponentOptions = {
  bindings: {
    timerange: '<'
  },
  controller: SysMonitorController,
  templateUrl: `...`
};

and then in a directly above it, I have components.ts

///<reference path="../../../../../../typings/index.d.ts"/>
import someServicesModule from '../services/someServices'; // <-- module
import { sysMonitorComponent } from "./sysMonitor/sysMonitor"; // <-- named component

/**
 * @ngdoc module
 * @name app.components
 */
export default angular
    .module('app.components', [
      someServicesModule
    ])
    .component('avrSystemMonitor', sysMonitorComponent)
    // ...
    .name;

And of course, one level above it, there is the main app.ts file which has, among others:

import componentsModule from './components/components'; // <-- I am still considering using 'index.js' (.ts) for the "glue" file in each directory.
// import other modules here
import { someConfigFunction } from './config';

/**
 * @ngdoc module
 * @name app
 */
angular.module('dosVisibility', [
    componentsModule,
    // other imported modules
  ])
  .config(['$compileProvider', function($compileProvider) {
    $compileProvider.debugInfoEnabled(false);
  }])
  .run(someConfigFunction);

The app file is the entry point (for webpack), so it doesn't really need an export. If it would have been a library, then the entry point would also have an export.

I find this pattern very useful. Additionally, exporting by name instead of defaults is pretty much the only way any IDE knows what you mean, even before you made the import. For example, in both VSCode and JetBrains IDEs, you can use an object from another file, and it will automatically add the import. It will not do it for "default" export since the names mean nothing before these are imported. Just for this, I think it's worth to use named exports.

I wonder what are the thoughts of the community about it.

bcarroll22 commented 8 years ago

I've not used a convention like this before. In every case if there's only one export from a file, we use the default export. Even when there are multiple exports we tend to pick the most sensible one as the default, and export all others as a named default.

I've never used VSCode or WebStorm, but I'm surprised they have such a hard time with default exports. The default imports don't break, right? You're just saying it can't auto import them? It should still know there's a default export from a module though.

import ConfigFunction from './config'

is just syntactic sugar for:

import { default as ConfigFunction } from './config'
nomaed commented 8 years ago

@bcarroll22 Actually I was wrong, WebStorm knows about default exports, if these are named objects. However, consider that you have a file with a service class, but there are some config consts perhaps or other meta-data that you want to make available. IMO it is nice that everything is explicitly exported:

// foo.js
export const FOO_DEFAULTS = {  };
export class FooService {  }

// bar.js
import { FooService, FOO_DEFAULTS } from './foo';

I am importing 2 objects that I explicitly defined in foo.js. I am not aliasing anything. It feels much more straight-forward to me.

And additionally, it's an invalid syntax to export a const as a default so I can't do

// error:
export default const MY_SETTINGS = {  };

// valid:
const MY_SETTINGS = {  };
export default MY_SETTINGS;

so there's more code to write, although it's insignificant ;)

But still, back to the first example, and what you said about import { default as SomeThing } from ..., that's exactly the point that I don't want to import default. I want to import the actual exported class, so for me import { MyClass } from './services/MyClass' makes much more sense, even if it's a single export.

bcarroll22 commented 8 years ago

I see what you're saying now. Seems like this really comes down to a preference thing. Most of the packages you'll see on npm are taking advantage of default exports, so I don't think you'll be seeing a lot of third party packages solely exporting named resources, so I imagine VSCode and WebStorm might still trip on them.

That being said, I know I've seen people like Dan Abramov (the writer of Redux) write this way:

const ConfigToExport = {
  // ...whatever
}

export default ConfigToExport

I tend to like that when I see it. Sometimes I think it's nice to define exports at the top and actually export at the bottom.

toddmotto commented 8 years ago

Closing off, feel free to open if you need to. Thanks for the conversation on this one. šŸ‘