Convert codebase to ES6 #1126

simison · 2019-11-15T20:59:56Z

Proposed Changes

Convert all code (server and client) to ES6:
- Manually fix a few var → let/const situations
- Remove use strict using https://github.com/5to6/5to6-codemod and enable no-strict rule
- Convert var → const/let (using eslint --fix and a few manual fixes)
- Other things that eslint --fix can do
Reorganise testutils to server and client subfolders for easier linting via separate server & client side overrides
Update Eslint config:
- Lint everything as es6
- Refactor to be more specific for each override section; each section has their own globals instead of keeping them together for the whole app
- Separate client-side and server-side, as well tests and app files.
- Use eslint:recommended as a base; remove rules in our custom set which would be duplicate with recommended.
- Add one-var linter rule since we're already touching all these lines. This should make e.g. automatically replacing require→import easier later on.
- Removed all redundant Eslint disabled rules

Testing Instructions

Please review .eslintrc.js — there are so many other changes that you might just want to skim through a bit and smoke test the site.
npm start works?
npm run lint green?
Confirm that clientside built bundles don't have const/let in sources and that they're ES5 code.

simison · 2019-11-24T14:31:31Z

There's one Eslint error that I didn't yet figure out of it's legit or not:

modules/references/server/controllers/reference.server.controller.js
  415:5  error  Possible race condition: `req.reference` might be reassigned based on an outdated value of `req.reference`  require-atomic-updates

At this line:

trustroots/modules/references/server/controllers/reference.server.controller.js

Line 417 in 51d4674

req.reference = formatReference(reference, isUserFrom);

Rule details: https://eslint.org/docs/rules/require-atomic-updates

simison · 2019-11-24T14:38:40Z

I'm marking this already ready for review; a couple pesky lint issues to fix but otherwise ready.

simison · 2019-11-24T14:39:37Z

.eslintrc.js

  'object-curly-spacing': [2, 'always'],
-  'one-var': [0, 'never'],


This is a new rule.

simison · 2019-11-24T14:40:07Z

.eslintrc.js

@@ -22,24 +22,19 @@ const defaultRules = {
  'no-caller': 2,
  'no-console': 2,
  'no-else-return': 0,
-  'no-empty-character-class': 2,


All (except one-var) removed rules are still applied because they're in eslint:recommend config. Just removing as duplicates.

mrkvon · 2019-11-26T16:08:08Z

@simison wrote:

There's one Eslint error that I didn't yet figure out of it's legit or not

There is an open issue for this behaviour on eslint. They'll remove it from eslint:recommended in the next minor release.

There is nothing racing in our case. We may just

// eslint-disable-next-line require-atomic-updates
req.reference = whatever()

mrkvon · 2019-11-26T16:35:58Z

Probably not related to this PR, but eslint-plugin-angular in package.json may want to go to devDependencies instead of dependencies.

mrkvon

Lots of linter work done! 👍

app boots
some eslint errors still need fixing
added some comments; some of them unrelated
after npm run build:prod my public/dist/main.js contains no let and const and await, but contains a few Promises. If the above file is part of the frontend build, then the code is not ES5 or I've overlooked something (like... is Promise defined with a polyfill somewhere?). (Promises are present since ES6)

So when these are resolved or proven not issue, I'll support merge of this PR.

.eslintrc.js

mrkvon · 2019-11-26T13:00:47Z

.eslintrc.js

-  'no-redeclare': 2,
-};
-
-const es2018rules = {


Bold move! 👍

simison · 2019-11-28T08:55:48Z

.eslintrc.js

+        angular: 1,
+      },
+      env: {
+        browser: true,


We actually might want to turn this browser env off and simply allow document and window globals.

That will stop nasty bugs like where someone uses find() and forgets to import it from lodash, causing window.find to be referenced and nothing would warn about it — it would just silently break.

There are some 700+ globals in browsers so little too permissive. 😁 Better be explicit about them.

Good for another PR.

By using https://github.com/5to6/5to6-codemod

https://eslint.org/docs/rules/one-var

To be replaced by 9c19102b

These are in server side overrids block and thus already linted.

This isn't critical — will be nice to fix some day but ok for now

Es we transform to promises code, we'll have plenty of these warnings with `req` object...

...these aren't enabled anywhere so no need to disable them explicitly.

simison · 2019-12-01T16:54:00Z

@mrkvon this no longer throws eslint warnings (I simply inline-ignored the two in bin script 421ae54 and disabled another rule in eslint since it'll likely pop up quite a bit when we transform more backend code to promises. c3f4ce9

Good for one more spin before merging!

mrkvon

A lot of good work on this one! 👍

site starts
tests pass
linter is happy

Added a question about strict mode. Besides that, this seems ready for a merge. 🚀

mrkvon · 2019-12-03T20:04:53Z

.eslintrc.js

@@ -50,197 +34,204 @@ const defaultRules = {
  }],
  'space-in-parens': [2, 'never'],
  'spaced-comment': [2, 'always'],
-  strict: 0,
+  strict: [2, 'never'],


What is the reasoning behind having this rule? Do we actually want to use sloppy mode? (I guess not?) Or do we address the issues of non-strict mode in a different way?

So ES6 modules are always in strict mode automatically, so the declaration is redundant. That said, we have heaps of old Angular code that would prolly benefit from having it, but in my understanding, we'll be safe just by nature of us modifying and adding mostly new ES6 code and removing old Angular "ES5" code.

I see. Makes sense.
We could still benefit from strict mode in api code and tests, couldn't we? AFAIK it uses require and module.exports for exporting and importing. Have we used strict mode until now?

nicksellen

I was a bit worried how other open PRs might rebase onto this one, I tried #1134 and it rebased cleanly, as did a couple of others.

I didn't look in detail tbh, but seems good! Can always tweak in the future if it doesn't seem quite right.

simison · 2019-12-04T18:13:36Z

Thanks, folks!

Yeah, if something gets merged without being rebased and introduces ES5, it's fine, we can jump in and those will be super simple fixes.

simison added [Status] in progress labels Nov 15, 2019

simison mentioned this pull request Nov 15, 2019

Fix eslint no-prototype-builtins issues #1127

Merged

simison force-pushed the update/eslint-es6 branch 3 times, most recently from 4fde13f to f74b3e7 Compare November 24, 2019 14:12

simison requested a review from mrkvon November 24, 2019 14:37

simison marked this pull request as ready for review November 24, 2019 14:38

simison commented Nov 24, 2019

View reviewed changes

.eslintrc.js

'object-curly-spacing': [2, 'always'],

'one-var': [0, 'never'],

Copy link

Contributor Author

simison Nov 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new rule.

simison commented Nov 24, 2019

View reviewed changes

simison requested a review from nicksellen November 24, 2019 14:51

mrkvon reviewed Nov 26, 2019

View reviewed changes

simison commented Nov 28, 2019

View reviewed changes

simison added 13 commits December 1, 2019 10:01

Update eslint config

b631a5c

Various fixes

868a984

Remove use strict

8161d28

By using https://github.com/5to6/5to6-codemod

Disallow grouping multiple variables

e48f3dd

https://eslint.org/docs/rules/one-var

eslint --fix

ccea752

Fix no-useless-escape lint issues

1730c9e

Remove rules already defined in eslint:recommend

4253224

Update eslint disables in two Angular files

20ade44

Reorganise testutils to server and client subfolders

3ebdf77

To be replaced by 9c19102b

Refactor Eslint config to be more specific for each override section

25c41c5

Remove eslint-ignored scripts folder from the config

5b37f3b

Remove reduntant config overrides in React section

b410ec1

These are in server side overrids block and thus already linted.

Ignore self lint error (false alarm)

835ff7f

simison force-pushed the update/eslint-es6 branch from 8895e1a to 835ff7f Compare December 1, 2019 08:02

simison added 4 commits December 1, 2019 12:19

Ignore lint errors in bin script

421ae54

This isn't critical — will be nice to fix some day but ok for now

Disallow strict in eslint

8456afd

Disable require-atomic-updates rule

c3f4ce9

Es we transform to promises code, we'll have plenty of these warnings with `req` object...

Remove reduntant eslint disable rules

a5b5135

...these aren't enabled anywhere so no need to disable them explicitly.

simison added [Status] ready for review and removed [Status] in progress labels Dec 1, 2019

Move eslint-plugin-angular to dev-dependencies

92abd16

mrkvon approved these changes Dec 3, 2019

View reviewed changes

nicksellen approved these changes Dec 4, 2019

View reviewed changes

simison merged commit 84d34ce into master Dec 4, 2019

simison deleted the update/eslint-es6 branch December 4, 2019 18:14

nicksellen mentioned this pull request Dec 13, 2019

ES6 migration #804

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert codebase to ES6 #1126

Convert codebase to ES6 #1126

simison commented Nov 15, 2019 •

edited

Loading

simison commented Nov 24, 2019 •

edited

Loading

simison commented Nov 24, 2019 •

edited

Loading

simison Nov 24, 2019

simison Nov 24, 2019

mrkvon commented Nov 26, 2019 •

edited

Loading

mrkvon commented Nov 26, 2019

mrkvon left a comment

mrkvon Nov 26, 2019

simison Nov 28, 2019 •

edited

Loading

simison commented Dec 1, 2019

mrkvon left a comment •

edited

Loading

mrkvon Dec 3, 2019 •

edited

Loading

simison Dec 4, 2019

mrkvon Dec 4, 2019

nicksellen left a comment

simison commented Dec 4, 2019

		'object-curly-spacing': [2, 'always'],
		'one-var': [0, 'never'],

Convert codebase to ES6 #1126

Convert codebase to ES6 #1126

Conversation

simison commented Nov 15, 2019 • edited Loading

Proposed Changes

Testing Instructions

simison commented Nov 24, 2019 • edited Loading

simison commented Nov 24, 2019 • edited Loading

simison Nov 24, 2019

Choose a reason for hiding this comment

simison Nov 24, 2019

Choose a reason for hiding this comment

mrkvon commented Nov 26, 2019 • edited Loading

mrkvon commented Nov 26, 2019

mrkvon left a comment

Choose a reason for hiding this comment

mrkvon Nov 26, 2019

Choose a reason for hiding this comment

simison Nov 28, 2019 • edited Loading

Choose a reason for hiding this comment

simison commented Dec 1, 2019

mrkvon left a comment • edited Loading

Choose a reason for hiding this comment

mrkvon Dec 3, 2019 • edited Loading

Choose a reason for hiding this comment

simison Dec 4, 2019

Choose a reason for hiding this comment

mrkvon Dec 4, 2019

Choose a reason for hiding this comment

nicksellen left a comment

Choose a reason for hiding this comment

simison commented Dec 4, 2019

simison commented Nov 15, 2019 •

edited

Loading

simison commented Nov 24, 2019 •

edited

Loading

simison commented Nov 24, 2019 •

edited

Loading

mrkvon commented Nov 26, 2019 •

edited

Loading

simison Nov 28, 2019 •

edited

Loading

mrkvon left a comment •

edited

Loading

mrkvon Dec 3, 2019 •

edited

Loading