Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate to ES6 #304

Closed
wants to merge 50 commits into from
Closed

Migrate to ES6 #304

wants to merge 50 commits into from

Conversation

vikaspotluri123
Copy link

@vikaspotluri123 vikaspotluri123 commented Oct 2, 2018

  • Remove usage of object.assign (dep)
  • Remove usage of util._extend (prefer native Object.assign)
  • Require Node v6+
  • var -> const / let
  • Object destructuring
  • Native ES6 Classes
  • Template strings

Refs #303

Todo

  • Fix tests
  • Migrate management folder

Node versions that support the ES6 migrations support native Object.assign
Migrating to ES6 breaks support for earlier versions
Potential issue: ManagementClient.deleteUserMultifcator is misspelled!
@@ -1,156 +1,125 @@
var expect = require('chai').expect;
Copy link
Author

Choose a reason for hiding this comment

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

Not sure why this file is so different, maybe precommit hook?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. Not sure why it wasn't formatted before..

test/auth/tokens-manager.tests.js Outdated Show resolved Hide resolved
@vikaspotluri123 vikaspotluri123 changed the title [WIP] Migrate to ES6 Migrate to ES6 Oct 2, 2018
@vikaspotluri123
Copy link
Author

https://github.com/vikaspotluri123/node-auth0/blob/es6/src/auth/index.js#L448 - I didn't migrate this to let / const since it's an unused variable - didn't want to forget to mention this - Should I remove it, or is the function call incorrect? There's another version of this on line 405

@luisrudge
Copy link
Contributor

If it's unused, feel free to remove it. Probably some copy/pasting mistake

@vikaspotluri123
Copy link
Author

  changePassword(data, cb) {
    var translatedData = {
      connection: data.connection,
      email: data.email || data.username,
      password: data.password
    };

    return this.database.changePassword(data, cb);
  }

Based on other functions, it feels like the function call should use translatedData, not data

@vikaspotluri123
Copy link
Author

I didn't want to fix the typo in https://github.com/vikaspotluri123/node-auth0/blob/es6/src/management/index.js#L1137 since it's part of the code rather than docs - thoughts?

@luisrudge
Copy link
Contributor

Let's not change behaviors in this PR. If the variable is unused, leave it as is

@luisrudge
Copy link
Contributor

Can't remove/fix that typo because it's part of the public API. It's already deprecated and it will be removed in the next major

@luisrudge
Copy link
Contributor

Although... This PR will be a breaking change anyway... 🤔 Let me think about this.

Regenerate lockfile to make bluebird resolve to the same version for this and rest-facade so we can use instanceof(Promise) in tests. Since it was regenerated, subdeps were updated and integrity hashes were added (new in Yarn v1.10.x)
@luisrudge luisrudge mentioned this pull request Oct 3, 2018
vikaspotluri123 and others added 2 commits October 4, 2018 09:47
Use es6-promisify in production (which is already required by subdeps) and bluebird in development for testing rest-facade
@vikaspotluri123
Copy link
Author

@luisrudge you're not waiting on me for anything, right? Just wanted to make sure 😄

@luisrudge
Copy link
Contributor

@vikasjayaram I didn't know it was done 😬 I'll try to do some testing this week. I still need to discuss with the team if this should be considered a breaking change or not (I think it is).

@luisrudge
Copy link
Contributor

hey @vikasjayaram - I'm sorry this is take more than expected. I'm not sure when this will get merged but I want to make it clear that I already contacted our hacktoberfest admin to make sure you get points for this one 🎉 Thanks so much for this PR.

@vikaspotluri123
Copy link
Author

No worries, thanks for the update 😄

@luisrudge
Copy link
Contributor

@vikaspotluri123 sorry I missed your username twice 😬

@luisrudge
Copy link
Contributor

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.

@luisrudge luisrudge closed this May 23, 2019
@vikaspotluri123 vikaspotluri123 deleted the es6 branch May 24, 2019 13:42
@mattiasnixell mattiasnixell mentioned this pull request Jul 28, 2020
5 tasks
@hornta hornta mentioned this pull request Oct 3, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants