Eslint: Sort import and require statements, fix no-unused-vars usage

[rsimha]

This PR makes the following eslint changes:

1. Introduces import sorting and auto-fixing via sort-imports-es6-autofix (https://www.npmjs.com/package/eslint-plugin-sort-imports-es6-autofix)
2. Introduces require sorting and auto-fixing via sort-requires (https://www.npmjs.com/package/eslint-plugin-sort-requires)
3. Fixes and guards against unintended file-level usage of no-unused-vars (in favor of line-level usage)

With this PR:

• gulp lint will flag errors for unsorted import and requires statements, and gulp lint --fix will fix them locally
• gulp presubmit will guard against unintended file-level usage of no-unused-vars, and suggest that the rule be used at the line-level

Fixes #13253

[rsimha]

/to @cramforce @erwinmombay

This is a large list of files, but the changes are fairly straightforward. I've gone through a self review to make sure everything looks good.

[rsimha]

/cc @dvoytenko @aghassemi @choumx @lannka for comments on whether there might be any drawbacks to sorting imports.

[erwinmombay]

i feel very uncomfortable doing auto sort fixing (i'm ok with suggesting/warning but not erroring and moving around everything), importing modules have side effects and order matters (not alpha sorting etc but module loading order)

[rsimha]

@erwinmombay To clarify, we don't automatically move around imports when gulp lint finds unsorted imports in a .js file. For that, you have to explicitly run gulp lint --fix, at which point, only the files being edited in the local branch will be changed. (See PR description for more details.)

Moreover, there is support for turning off import-sorting errors in a file where there's good reason to maintain a different order. See https://github.com/ampproject/amphtml/blob/master/3p/integration.js#L63 for example.

Does this alleviate your concern?

[erwinmombay]

if we didn't change the sort order of a few hundred files for the core runtime then yes. I guess we'll see in a week when this hits canary

[erwinmombay]

Honestly Im sure im just being paranoid, but we don't have enough integration tests coverage for this sort of change for me to feel confident (im sure the changes of an error are really low, like really low. only log.js really does global black magic side effect that would be obvious right away if it blew up)

[rsimha]

@erwinmombay You make a fair point.

I merged this PR after asking around the team this morning. The responses I received were all in favor of sorting. It turns out that the original intent all this time has been to keep the imports sorted, but since it was a manual process, they didn't stay that way.

Files in which the imports were explicitly grouped non-alphabetically were left out of this PR via an eslint rule exception. (They appeared to be more about code organization than module loading order.)

In addition, the research I did on module loading suggested that ES6 modules are loaded asynchronously, so declaration order doesn't strictly determine loading order. https://stackoverflow.com/questions/35551366/what-is-the-defined-execution-order-of-es6-imports

I appreciate your concern though. One option is to wait and watch for the next canary. In any case, I'm ccing @cramforce in case he thinks this is a large enough concern to cause us to revert this PR.

[jridgewell]

In addition, the research I did on module loading suggested that ES6 modules are loaded asynchronously, so declaration order doesn't strictly determine loading order.

That's not true. Imports are async (kinda, we'll see what Node does) in browsers, but the execution order is well-defined. We're currently debating this in TC39, but my general feeling is this will not change.

[rsimha]

@jridgewell What I meant to say was that module load order among imports in a file is undefined, even though all modules are loaded before running the code in the file that's doing the importing. (Unless this too is incorrect?)

Also, is this enough reason to revert this change, given the original intent of keeping our imports in order?

[dreamofabear]

I think as long as we don't muck with imports that look like import './<file>' we should be okay. That's easily grep-able too.

What black magic does log.js do? I thought only polyfills like document-register-element have side effects.

[jridgewell]

Yes, that's incorrect. Given:

// main.js
import {a} from './a.js';
import {b} from './b.js'

The files a.js and b.js can be fetched in any order (async), but a must evaluate before b. Both must evaluate before the body of main can (note, there are cycles that can muddle with this, but it's 99% true). We discussed sibling order execution in the last two meetings, because we're trying to figure out a path forward for Node.js.

is this enough reason to revert this change

No, probably not. I verified polyfills.js was still included first, which is the only case I'm concerned with.

log.js

It stores in a global variable. As long as the constructors are called in the main binary, everything will work correctly.

[rsimha]

@jridgewell Thanks for that explanation. It's worth mentioning that this PR doesn't change the order that's laid out in gulpfile.js, which is where the logic that includes polyfill.js lives.

I'm going to leave this PR as it is, and revisit this discussion if we encounter any issues.

Thanks for the comments, everyone!

[erwinmombay]

@rsimha-amp i don't think that is correct, the import order for polyfills.js is defined in src/amp.js and 3p/integration.js. It just so happens it is still the first one, but that could have been changed if for example there was a module import like import './a.js' (if i understand correctly how the sorter works)

[erwinmombay]

also im surprised at that stackoverflow answer...

[jridgewell]

also im surprised at that stackoverflow answer...

It's an unspecified convention. TC39 didn't want to spec the module loader, so we couldn't spec the evaluation order.

[rsimha]

@erwinmombay Aha, I now see what you meant (https://github.com/ampproject/amphtml/blob/master/src/amp.js#L21). (My last comment was referring to the build order in gulpfile.js.)

Yes, ./polyfill is retained as the first import in amp.js because the ordering is:

1. import './file';
2. import {Class} from './foo';
3. import {a, b, c} from './bar';

If it's imperative that ./polyfill remain at the top of amp.js, we can force eslint to leave it there, and not complain if later on, it happens to precede an import that's alphabetically after polyfill. I'll send out a PR to bake this in.

Edit: Added safeguards for polyfill.js in #13277.