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

Add no-namespace rule #239

Merged
merged 1 commit into from
Apr 12, 2016
Merged

Add no-namespace rule #239

merged 1 commit into from
Apr 12, 2016

Conversation

singles
Copy link
Contributor

@singles singles commented Apr 7, 2016

no-namespace

Reports if namespace import is used.

Rule Details

Valid:

import defaultExport from './foo'
import { a, b }  from './bar'
import defaultExport, { a, b }  from './foobar'

...whereas here imports will be reported:

import * as foo from 'foo';
import defaultExport, * as foo from 'foo';

Motivation:

One should only import names which are needed in current scope, nothing more.

Additonally, users of tools like rollup.js or webpack 2.0 which support tree shaking would benefit in a smaller bundle size because unused exports that are not imported can be eliminated.

If all names are required in addition to separate named exports, developer might export default object with all required members:

export const FOO = 1;
export const BAR = 2;

export default {
    FOO,
    BAR
}

@benmosher
Copy link
Member

FWIW: if there are no dynamic references to the namespace, you can still do tree-shaking. I don't believe there should be any performance impact to using a namespace import vs. individual names/default.

I am up for including this rule, but I think the motivation section of the doc should be revised to remove that language and it should be listed in the README under the Style guide list of rules?

@lo1tuma
Copy link
Contributor

lo1tuma commented Apr 8, 2016

@benmosher There is not always a way to statically analyze all references, for example:

import * as foo from './foo';
import thirdPartyLibrary from 'third-party';

thirdPartyLibrary.doSomething(foo);

@benmosher
Copy link
Member

@lo1tuma fair enough. That would be easy enough to lint explicitly, though. I can imagine some more nuanced enforce-tree-shaking rule that reports anything that compromises static analysis:

  • passing namespaces as a parameter (as you have shown)
  • dynamic references (foo[someKey])
  • etc.

I just don't want folks getting the idea that imported namespaces themselves compromise analysis, which I think the current state of the "Motivation" section in this PR pretty strongly implies.

Also "One should only import names which are needed in current scope, nothing more." is a pretty opinionated statement, IMO.

So I would be inclined to turn "Motivation" into

## When Not To Use It
If you want to use namespaces, you don't want to use this rule.

@lo1tuma
Copy link
Contributor

lo1tuma commented Apr 8, 2016

@benmosher Agreed, it makes sense to have a separate rule which explicitly enforces patterns that can be optimized by tree-shaking.

@singles
Copy link
Contributor Author

singles commented Apr 8, 2016

OK, I've updated rule docs and moved rule to the style guide section in README.md.

Thanks for your feedback guys :)

@benmosher
Copy link
Member

Looks good! Thanks!

@benmosher
Copy link
Member

Oh actually, could you add a blurb under the Unreleased section of CHANGELOG.md?

@singles
Copy link
Contributor Author

singles commented Apr 8, 2016

@benmosher no problem, done.

@benmosher benmosher merged commit 6293364 into import-js:master Apr 12, 2016
@benmosher
Copy link
Member

Awesome, thanks!

@gajus
Copy link
Contributor

gajus commented Apr 30, 2016

@singles Okay. The rule is built on a premise that discouraging use of import all will enable tree-shaking. However, file size is not the only concern. For the most part, importing just certain properties makes the code easier to read, e.g.

import {FOO, BAR} from './config';

// ...
// Do something with FOO, BAR

is a lot easier to scan than

import config from './config';

// ...
// Do something with config.FOO, config.BAR

However, at the same time, I might need to import the entire config, e.g. to serialise the config.

This rule encourages to use export default whenever there is at least one instance in code where there is need to import the entire config. In effect, promoting use of the pattern in the latter example.

@singles singles deleted the no-namespace-rule branch April 30, 2016 13:58
@singles
Copy link
Contributor Author

singles commented Apr 30, 2016

@gajus tree shaking is one of the examples. Also, in our team we're considering importing only needed stuff as good practice.

When namespace imports are disallowed, author of library/module can decide, if he want's to expose whole object or not.

Then

export const FOO = 123;
export const BAR = 456;

allows you to import only specific members (and whole module using *), but if library author decides that they would also like to export whole stuff, they can always do:

export const FOO = 123;
export const BAR = 456;

export default { FOO, BAR };

And of course, you can always use //eslint-disable

@gajus
Copy link
Contributor

gajus commented Apr 30, 2016

@singles that is a good example:

export const FOO = 123;
export const BAR = 456;

export default { FOO, BAR };

Thank you. This made me change the setting for no-namespace from 0 to 2. : )

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

Successfully merging this pull request may close these issues.

4 participants