Migrate to ES6

[vikaspotluri123]

Howdy!

As part of Auth0 ❤️ Hacktoberfest I was thinking of migrating this library to ES6 - are there any objections and / or limitations regarding this?

The only possible downside I see with this is this would drop support for Node v4, although it's been EoL since April 🤔

[luisrudge]

@vikaspotluri123 that would be amazing, yes. The only hard limitation is that it has to work in the oldest LTS release, which I believe is 6.x now.

Do you plan to use any automated tools to do this? I remember testing https://lebab.io/ once in another project and it kinda worked ok (for the most part).

I think we can start by picking a few files and converting them and then we'll go from there. What do you think?

[vikaspotluri123]

I was planning on doing both manual and automated 😄

A lot of my OSS contribution is into tryghost/ghost, and there was a link to esnext which I was going to try

[luisrudge]

Cool. I'd try with lebab first, since it's still under development and esnext has no activity in the past 7 months.

[vikaspotluri123]

Sounds good! Do you prefer liberally let or constantly const?

// Both liberally let and constantly const
const doTheMath = require('./do-the-math');

// liberally let
const liberal = (a, b) => {
	let retVal = doTheMath(a + b);
	return retVal;
};

// constantly const
const constant = (a, b) => {
	const retVal = doTheMath(a + b);
	return retVal;
}

[luisrudge]

const when possible

[vikaspotluri123]

I've migrated src/utils.js, changes can be seen in the diff. See the diff with whitespace included

[luisrudge]

Pretty cool. Let's see how the auth folder looks after the migration 😬 Also, don't forget to run the tests and see if everything is ok.

[vikaspotluri123]

I'm running tests locally, and ~10 are failing with nock-related errors. My hope is it's just an OS-compat issue since it's been failing since before I made changes.

For the RetryRestClient, converting it to an ES6 class causes some tests to fail since it's not invoked with the new keyword. Since this seems to be an internal class, are there any issues with updating the tests?

Also, since node v6+ supports Object.assign, I'm removing the object.assign dependency

[luisrudge]

I'm running tests locally, and ~10 are failing with nock-related errors. My hope is it's just an OS-compat issue since it's been failing since before I made changes.

🤔 Weird. What errors are you seeing?

For the RetryRestClient, converting it to an ES6 class causes some tests to fail since it's not invoked with the new keyword. Since this seems to be an internal class, are there any issues with updating the tests?

Do you have an example?

Also, since node v6+ supports Object.assign, I'm removing the object.assign dependency

ok

[vikaspotluri123]

🤔 Weird. What errors are you seeing?

  5) should use the default headers
Unhandled rejection Error: Nock: No match for request POST https://tenant.auth0.com/users/github%7C12345/impersonate {"client_id":"TEST_CLIENT_ID","token":"API V1 TOKEN","protocol":"oauth2","impersonator_id":"auth0|12345"}
    at end (/mnt/c/code/git/node-auth0/node_modules/nock/lib/request_overrider.js:259:17)
    at OverriddenClientRequest.RequestOverrider.req.end (/mnt/c/code/git/node-auth0/node_modules/nock/lib/request_overrider.js:170:7)
    at Request.end (/mnt/c/code/git/node-auth0/node_modules/request/request.js:1515:14)
    at end (/mnt/c/code/git/node-auth0/node_modules/request/request.js:548:16)
    at Immediate._onImmediate (/mnt/c/code/git/node-auth0/node_modules/request/request.js:575:7)
    at runCallback (timers.js:810:20)
    at tryOnImmediate (timers.js:768:5)
    at processImmediate [as _immediateCallback] (timers.js:745:5)

Do you have an example?

  13) RetryRestClient should raise an error when maxRetries is negative:
     AssertionError: expected [Function: bound RetryRestClient] to throw 'ArgumentError' but 'TypeError: Class constructor RetryRestClient cannot be invoked without \'new\'' was thrown
      at Context.<anonymous> (test/retry-rest-client.tests.js:29:28)

Once I migrate this file, I'm going to create a PR to test w/ CI

[vikaspotluri123]

On a separate note, I'm also fixing typos as I see them, partially because my editor doesn't like them 😅

[vikaspotluri123]

For now, I've locally migrated the auth folder and the src/*.js folder, I'll migrate the management folder tomorrow (hopefully). 30 failing tests atm w/ a good portion failing because of the new keyword issue

[vikaspotluri123]

I'm running tests locally, and ~10 are failing with nock-related errors. My hope is it's just an OS-compat issue since it's been failing since before I made changes.

Seems to be so, most of the failing tests on CircleCI relate to expected failures (i.e. new keyword)

FWIW My env is:

Windows 10 1803 Build 17134.285
Windows Subsystem for Linux (since windows doesn't have find, it has to be run in WSL)
Yarn v1.10.1
Node v8.12.0

[luisrudge]

Thanks @vikasjayaram for all your effort in this issue. We have big plans for Management SDKs and, sadly, we'll have to drop this PR in favor of what's next.