Use eslint to catch import mistakes and style nits

[kumar303]

Fixes mozilla/addons#9804

• Added checks for missing imports and bad named imports. Yay!
• Added a check to make sure we don't accidentally import a named export as a default. For example, consider this:

import Foo from './thing';

If thing.Foo actually exists then this could have been a typo. The developer probably meant:

import { Foo } from './thing';

I feel like we were abusing this to get at raw components without their redux decorators. Thus, I needed to rename all non-decorated components from TheComponent to TheComponentBase. If this makes things awkward, just let me know and we can change it back.

• Added a rule for PEP8 style import ordering (not alphabetized though)
• Added a check to make sure dev dependencies aren't used in production code. This caught some bugs!
• Relative test imports can no longer be used. Technically these work because karma runs from the root we set NODE_PATH and technically we could allow it in the linter by adding cwd to the import resolver. However, I think this is bad practice because it makes other imports confusing. For example, is import config getting the one from node_modules or the one at ./config ? I don't think it's too gnarly to use relative imports in the test suite (like ../../helpers). UPDATE we decided to allow relative imports in the test code.

[mstriemer]

I'm on PTO without my laptop so I'll have to pass this review to someone else.

It has conflicts though.

[muffinresearch]

This is awesome. Let me know when the conflicts are resolved and I'll take a closer look.

[muffinresearch]

Is there a way to not need non-relative imports here - I see we wouldn't want cwd configured in the linter though. We could explicity add the project root couldn't we?

Using relative imports gets really messy fast with all the ../../../../. Also it makes test code less easily portable.

[muffinresearch]

If we directly add the project root can we do that just for tests?

[kumar303]

We can add the project root but it feels a bit risky to me. If you look at our project root we have a lot of directories. For example, ./config is a directory but we also use the config from node_module as well. This actually confuses the linter in some places (which may be a bug). It doesn't cause errors though so we could do it.

The other alternative is to push tests into another subdirectory like tests/testlib and import everything from testlib.

[muffinresearch]

Yeah node-config is a bit magical in the way it works.

[muffinresearch]

I'm not sure I understand the need to suffix components with Base - do you have an clearer example of where this would be an issue.

Historically our usage was like this:

import WhateverComponent from  'disco/components/whatever';

and that's going to get the default exported component. But if you want the un-wrapped component we do:

import { WhateverComponent } from  'disco/components/whatever';

So am I right in thinking that adding Base is mainly about making the distinction a bit clearer?

[kumar303]

So am I right in thinking that adding Base is mainly about making the distinction a bit clearer?

It really comes down to this rule: https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/no-named-as-default-member.md

Do we want to keep it? If we do, we can't name components the same as the default variable we use to import them with.

We can do this just fine:

import Addon from 'amo/components/addon';

but we can't name the undecorated class Addon because the linter will think you made a mistake and really meant to do:

import { Addon } from 'amo/components/addon';

I can see how this kind of mistake might happen so the rule seems useful. To use the rule, we have to rename all the base components to something like AddonBase and do this:

import { AddonBase } from 'amo/components/addon';

This is actually kind of nice because it makes it unambiguous that you meant to import a base component for a test, one that is not decorated.

[muffinresearch]

This is actually kind of nice because it makes it unambiguous that you meant to import a base component for a test, one that is not decorated.

Ok that works for me - keep it in.

[kumar303]

ooh, I can maybe solve the test import path issue by adding the project root path only to tests/.eslintrc and not to the main .eslintrc

[muffinresearch]

It's worth bearing in mind that the server code gets the root in the NODE_PATH. Maybe we can get rid of that? See package.json.

[kumar303]

d'oh! NODE_PATH is not recognized. Once that's fixed we could switch to that. However, adding the entire project root to NODE_PATH does seem a bit risky to me.

[kumar303]

To clarify: I thought that maybe we could use NODE_PATH for the linter to make it match runtime. I think you were suggesting to remove it altogether though. I would be fine with that. Until then, I just put test helper imports back to where they were (e.g. import { foo } from 'tests/client/helpers'). I did this by adding the project root path to tests/.eslintrc.

[kumar303]

@muffinresearch thanks for the review. If anything breaks while I'm on PTO next week, you know what to do :) (and sorry). I think it should be fine.