refactor to es6 syntax with classes #665

hornta · 2021-10-03T23:59:34Z

Changes

Summary of changes:

Refactor existing prototypes to ES6 classes. This includes getting rid of the wrapPropertyMethod which hides methods on these classes. Now it's possible for IDES to autodetect these methods.
Added eslint to catch mistakes during refactoring and to maintain es6 syntaxes in the future.
Improved formatting of JSDoc comments and removed some unnecessary tags like @class, @function and @memberof now when the classes are real classes and have real methods. All class methods which are forwarding all of their arguments (...args) still has their respective JSDoc @param tags which technically is wrong but since the underlying library works the way it does then it will have to be like that for now.
Removed moment devDep
Updated some testing dependencies like mocha and sinon.
Added node 16 to the ci test/build matrix.
Got rid of dependency on es6-promisify by using native Promise objects, much like es6-promisify did.
Got rid of the >10s long test should expire access token from cache by the expires_in setting by using sinons fake timers to advance time.

References

Testing

Test logic has not been altered other than conforming to the linting rules and es6 class syntax.

It tooks me some time to figure out why the tests were not working initially so if any newcomers see this you will need to install OpenSSL 1 (NOT 3 because that is not yet supported by the pem package) and make sure you remove the CRLF EOL in users.json test data file.

Checklist

I have read the Auth0 general contribution guidelines
I have read the Auth0 Code of Conduct
All existing and new tests complete without errors

adamjmcgrath · 2021-10-04T08:58:20Z

👋 Hi @hornta - you've clearly done a lot of work here, so many thanks

My concern is whether we need a breaking change for dropping support for older, albeit EOL, versions of Node. If that's the case we may be reluctant to take this PR if we don't want to do a major release. But I'll check with the team and get back to you.

hornta · 2021-10-04T14:02:46Z

👋 Hi @hornta - you've clearly done a lot of work here, so many thanks

My concern is whether we need a breaking change for dropping support for older, albeit EOL, versions of Node. If that's the case we may be reluctant to take this PR if we don't want to do a major release. But I'll check with the team and get back to you.

Yea I agree that it should maybe be a breaking change since there is not really a good way of determining the minimum supported node version at this moment. However there are a bunch of object spread syntaxes in the code base right now which came to be supported after classes were supported in node so introducing classes should not cause a major version bump but I think it's quite good to define the supported engines.

…ject.

hornta · 2021-10-06T12:23:09Z

@adamjmcgrath The CI tests doesn't want to start for some reason? Can I trigger then manually?

frederikprijck · 2021-10-06T12:24:33Z

@hornta , our CI doesnt run on forks so no worries about it.

adamjmcgrath · 2021-10-19T09:37:22Z

@hornta - thanks for raising this. Happy to review once you resolve the conflicts.

I've set up the CI to run on Node versions 8, 10, 12, 14 - as long as it passes on those (which es2017 should do) it should be fine.

I think it's quite good to define the supported engines.

I agree, but I don't want to introduce a breaking change, so could you remove the engine requirements

hornta · 2021-10-19T10:03:26Z

@hornta - thanks for raising this. Happy to review once you resolve the conflicts.

I've set up the CI to run on Node versions 8, 10, 12, 14 - as long as it passes on those (which es2017 should do) it should be fine.

I think it's quite good to define the supported engines.

I agree, but I don't want to introduce a breaking change, so could you remove the engine requirements

Thanks. I removed the engine requirements and resolved the merge conflicts. On an unrelated note, do you mind if node 16 is added as well since it's what I've been developing on and it seems to work great or what have your team decided?

adamjmcgrath · 2021-10-19T10:17:03Z

Thanks. I removed the engine requirements and resolved the merge conflicts.

Thanks, I'll take a look over this over the next day or so

On an unrelated note, do you mind if node 16 is added as well since it's what I've been developing on and it seems to work great or what have your team decided?

Sure, feel free to add it to the matrix https://github.com/auth0/node-auth0/blob/master/.circleci/config.yml#L44

adamjmcgrath · 2021-10-20T10:02:29Z

@hornta - you have some lint failures on CI https://app.circleci.com/pipelines/github/auth0/node-auth0/845/workflows/149addf8-9501-4952-af9e-c58c621a0a53/jobs/1213

hornta · 2021-10-20T11:25:39Z

@hornta - you have some lint failures on CI https://app.circleci.com/pipelines/github/auth0/node-auth0/845/workflows/149addf8-9501-4952-af9e-c58c621a0a53/jobs/1213

I resolved the lint failres by setting the engines field in package.json to 8.3.0 since that's where node came to support object spread syntax.

However I noticed that ESLint failed as well for node 8 and 10 since ESLint only support node 12 and onward so I had to create a parallell job just for node 8 and 10 which just doesn't run the ESLint step. I don't know if it can be made prettier in the config.yml but I have limited experience with CircleCI config. It's kinda ugly to have to repeat the complete job but just removing the ESLint step.

I also noticed that the tests were failing for both node 8 and 10 because I updated mocha from major 6 to 9 and between these versions mocha dropped support for both 8 and 10 so I had to revert back the mocha upgrade to what we had before which is (6.1.0).
However by downgrading mocha, most of my tests failed because of a bug in mocha where it didn't handle rejected promises correctly when using async/await and that bug was fixed in mocha@8 so I tried to downgrade to mocha@8. However between [email protected] and mocha@8 it's dependency on yargs-parser dropped support for node 8 which is now causing our node 8 test runs to fail.

I don't feel like reverting all of the failing tests because of the bug which was fixed in a future mocha version so I guess this PR must then be on hold until node-auth0 decides to make a new major and drop support for 8 and hopefully 10 as well since most of our deps doesn't seem to support these.

…sion 8 and 10

adamjmcgrath · 2021-10-20T16:01:28Z

Thanks for your work on this @hornta, I'll take a look to see if there are any other options and get back to you

hornta · 2021-10-20T17:13:20Z

Thanks for your work on this @hornta, I'll take a look to see if there are any other options and get back to you

Great. I aim to not let this rot away so I will keep this branch up to date because when this is in then it will be easier converting to typescript because it uses classes then.

adamjmcgrath · 2021-10-22T11:21:43Z

@hornta

npx mocha@6 --reporter spec ./test/**/*.tests.js is passing for me on Node 8. Could you conditionally target that instead of npm test for the Node 8 job?

hornta · 2021-10-22T11:54:17Z

@hornta

npx mocha@6 --reporter spec ./test/**/*.tests.js is passing for me on Node 8. Could you conditionally target that instead of npm test for the Node 8 job?

Thanks, it seems to not throw errors now which is nice.

adamjmcgrath

Looks good, couple of things

Haven't looked through the tests yet

.eslintrc.js

package.json

adamjmcgrath · 2021-10-22T13:42:52Z

src/auth/OAUthWithIDTokenValidation.js

+    this.clientSecret = options.clientSecret;
+    this.domain = options.domain;
+    this.supportedAlgorithms = options.supportedAlgorithms || ['HS256', 'RS256'];
+    this._jwksClient = (options.jwksClient || jwksClient)({


What's the issue with using proxyquire?

I think I didn't see a need for it , also it doesn't work when using ESM(which this library doesn't yet but hopefully will sometime :-)). I would want to remove that dependency completely because I think if it's needed then simple dependency injection can be used as I've done here.

I'd rather avoid adding new options to this class just for testing.

also it doesn't work when using ESM(which this library doesn't yet but hopefully will sometime :-))

If it comes to that we can always switch to something like jest, which has good options for stubbing esm

src/management/ConnectionsManager.js

adamjmcgrath · 2021-10-26T15:23:48Z

src/management/RolesManager.js

+   * var params = {
+   *   roleId: 'ROLE_ID',
+   *   per_page: 50,
+   * @param params


these should be removed

adamjmcgrath · 2021-10-26T15:53:59Z

src/management/UsersManager.js

+   * Get a list of organizations for a user.
+   *
+   * @example
+   * @param {...any} args


adamjmcgrath · 2021-10-26T15:54:18Z

src/management/UsersManager.js

   *
-   * @type {external:RestClient}
+   * @example
+   * @param {...any} args


adamjmcgrath · 2021-10-26T15:54:38Z

src/management/UsersManager.js

+   *
+   * @example
+   * var params = { id: USER_ID };
+   * @param {...any} args


adamjmcgrath · 2021-10-26T15:54:57Z

src/management/UsersManager.js

+   *
+   * @example <caption>
+   *   This method takes an optional object as first argument that may be used to
+   * @param {...any} args


Co-authored-by: Adam Mcgrath <[email protected]>

adamjmcgrath · 2021-10-29T09:49:05Z

src/auth/OAUthWithIDTokenValidation.js

+    this.clientSecret = options.clientSecret;
+    this.domain = options.domain;
+    this.supportedAlgorithms = options.supportedAlgorithms || ['HS256', 'RS256'];
+    this._jwksClient = (options.jwksClient || jwksClient)({


I'd rather avoid adding new options to this class just for testing.

also it doesn't work when using ESM(which this library doesn't yet but hopefully will sometime :-))

If it comes to that we can always switch to something like jest, which has good options for stubbing esm

adamjmcgrath · 2021-10-29T09:50:58Z

src/management/ManagementTokenProvider.js

+    this.authenticationClient = new AuthenticationClient(authenticationClientOptions);
+
+    const self = this;
+    this.getCachedAccessToken = memoizer({


You can use built in promisify - you don't need es6-promisify

hornta · 2021-10-30T11:09:01Z

I'd rather avoid adding new options to this class just for testing.

If it comes to that we can always switch to something like jest, which has good options for stubbing esm

Alright so I reverted the changes so that proxyquire are used now. I now know why I wanted to remove proxyquire and it was because proxyquire throw an error when I tried to stub a sinon.spy()(because it's missing a .prototype property) and I don't know why it's like that, it must have been because of some updated dependency. Whatever I fixed it by attaching an empty prototype object myStub.prototype = {}. Not the prettiest but it doesn't throw now.

adamjmcgrath · 2021-11-19T09:45:16Z

Thanks for your work on this @hornta

I'm going to leave merging this until after the thanksgiving break - but I'll do it up shortly after

adamjmcgrath · 2021-11-19T09:47:14Z

FYI the coverage changes are as a result of changing a bunch of wrapPropertyMethod's to functions with body's

adamjmcgrath · 2021-11-29T09:10:43Z

Thanks @hornta - will do a release this week

refactor to es6 syntax with classes

ee4e8ea

hornta added 7 commits October 4, 2021 16:18

Refactored ConnectionsManager to class

54f8c97

Refactored ManagementTokenProvider to use es6 class

8b94b1a

Got rid of dependency on es6-promisify by using the native Promise ob…

3da4f1c

…ject.

Upgrade docker node image to 12

6befd38

Add some linting to aid in working with promises

b1c1639

Converted the remaining prototypes to classes

e222f32

Refactor test to use async/await for increased readability

fb49243

hornta marked this pull request as ready for review October 5, 2021 10:05

hornta requested a review from a team as a code owner October 5, 2021 10:05

Merge remote-tracking branch 'upstream/master' into es6

8f3b0b2

Merge branch 'master' into es6

caf4740

hornta added 3 commits October 19, 2021 11:42

remove engine requirements

7ed1742

Merge branch 'es6' of https://github.com/hornta/node-auth0 into es6

93f2bf7

Merge remote-tracking branch 'upstream/master' into es6

95d9c64

adamjmcgrath and others added 3 commits October 19, 2021 12:33

Merge branch 'master' into es6

3f6ce91

add node v16 to ci

eef7e91

Merge branch 'es6' of https://github.com/hornta/node-auth0 into es6

dd4085d

set minimum support node version

1db9d4b

hornta added 2 commits October 20, 2021 13:33

run ci on node 8 and 10 without eslint

042003a

remove eslint for the no-lint script, duh...

6e2a0e7

hornta added 2 commits October 20, 2021 13:40

disable running test on 8 and 10 because mocha has no support for ver…

79f5423

…sion 8 and 10

test downgrading mocha to 8

7afa69f

run eslint conditionally in job

4413912

hornta added 2 commits October 22, 2021 13:40

update husky hooks placement

1f301ac

run the tests on node 8 in mocha 6 with npx

e45438f

adamjmcgrath suggested changes Oct 26, 2021

View reviewed changes

hornta and others added 4 commits October 28, 2021 20:55

Update src/management/ConnectionsManager.js

9dd4d30

Co-authored-by: Adam Mcgrath <[email protected]>

Update src/management/LogsManager.js

64146b8

Co-authored-by: Adam Mcgrath <[email protected]>

cleanup

18f79a7

Merge branch 'es6' of https://github.com/hornta/node-auth0 into es6

738e065

adamjmcgrath suggested changes Oct 29, 2021

View reviewed changes

hornta added 2 commits October 30, 2021 13:02

Revert back to using proxyquire and

4dfb600

Made the getCachedAccessToken a promise again

9f395f2

adamjmcgrath previously approved these changes Nov 19, 2021

View reviewed changes

Merge branch 'master' into es6

f50f779

adamjmcgrath dismissed their stale review via f50f779 November 19, 2021 10:15

adamjmcgrath approved these changes Nov 19, 2021

View reviewed changes

adamjmcgrath merged commit 08d2302 into auth0:master Nov 29, 2021

This was referenced Jan 26, 2022

Release v2.38.0 #694

Closed

Release v2.38.0 #695

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor to es6 syntax with classes #665

refactor to es6 syntax with classes #665

hornta commented Oct 3, 2021 •

edited

Loading

adamjmcgrath commented Oct 4, 2021

hornta commented Oct 4, 2021

hornta commented Oct 6, 2021

frederikprijck commented Oct 6, 2021

adamjmcgrath commented Oct 19, 2021

hornta commented Oct 19, 2021

adamjmcgrath commented Oct 19, 2021

adamjmcgrath commented Oct 20, 2021

hornta commented Oct 20, 2021 •

edited

Loading

adamjmcgrath commented Oct 20, 2021

hornta commented Oct 20, 2021 •

edited

Loading

adamjmcgrath commented Oct 22, 2021

hornta commented Oct 22, 2021 •

edited

Loading

adamjmcgrath left a comment

adamjmcgrath Oct 22, 2021

hornta Oct 28, 2021

adamjmcgrath Oct 29, 2021

adamjmcgrath Oct 26, 2021

adamjmcgrath Oct 26, 2021

adamjmcgrath Oct 26, 2021

adamjmcgrath Oct 26, 2021

adamjmcgrath Oct 26, 2021

adamjmcgrath Oct 29, 2021

adamjmcgrath Oct 29, 2021

hornta commented Oct 30, 2021

adamjmcgrath commented Nov 19, 2021

adamjmcgrath commented Nov 19, 2021

adamjmcgrath commented Nov 29, 2021

refactor to es6 syntax with classes #665

refactor to es6 syntax with classes #665

Conversation

hornta commented Oct 3, 2021 • edited Loading

Changes

References

Testing

Checklist

adamjmcgrath commented Oct 4, 2021

hornta commented Oct 4, 2021

hornta commented Oct 6, 2021

frederikprijck commented Oct 6, 2021

adamjmcgrath commented Oct 19, 2021

hornta commented Oct 19, 2021

adamjmcgrath commented Oct 19, 2021

adamjmcgrath commented Oct 20, 2021

hornta commented Oct 20, 2021 • edited Loading

adamjmcgrath commented Oct 20, 2021

hornta commented Oct 20, 2021 • edited Loading

adamjmcgrath commented Oct 22, 2021

hornta commented Oct 22, 2021 • edited Loading

adamjmcgrath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hornta commented Oct 30, 2021

adamjmcgrath commented Nov 19, 2021

adamjmcgrath commented Nov 19, 2021

adamjmcgrath commented Nov 29, 2021

hornta commented Oct 3, 2021 •

edited

Loading

hornta commented Oct 20, 2021 •

edited

Loading

hornta commented Oct 20, 2021 •

edited

Loading

hornta commented Oct 22, 2021 •

edited

Loading