Proposal: Remove Use of getAuthenticateData

[rtablada]

While I understand the value of getAuthenticateData as a way of sanitizing data, it does cause a lot of confusion and misdirection for users while providing little in terms of extensibility.

https://github.com/jpadilla/ember-simple-auth-token/blob/master/addon/authenticators/token.js#L146-L153

Also it does not use Ember.get so you could lose the potential value of unwrapping objects, proxies or computed properties.
This is something that could have been useful in a sanitization/unwrapping function.

Passing credentials directly as an object would be clearer from a use perspective and would really slim down confusion around identificationField and passwordField config since they wouldn't be needed anymore.
This also makes the library more flexible for use with other systems which may require more than two fields.
I have worked in projects where other session IDs are passed in along with email/password to the token endpoint and this would require extending and rewriting the getAuthenticateData or even the entire makeRequest function.

---

Example in an app may have a controller with the following code:

this.session.authenticate('authenticator:token', { identification: this.email, password: this.password })

If after API development the JWT endpoint requires email and password instead of the configured identification and password then the thought would be to change the code to:

this.session.authenticate('authenticator:token', { email: this.email, password: this.password })

But instead the configuration for identificationField would need to be changed to email.

---

In comparison if there was no getAuthenticateData function or call, then it becomes much more clear and simple to pass in the fields required by the API being used.
Th code change listed be required and users wouldn't have to know about configuration for identificationField or passwordField at all, and they would have more flexibility.

---

Downside:

Removing getAuthenticateData would potentially break applications where credentials aren't explicitly passed in (for instance using this as the credentials in a controller/component and letting getAuthenticateData remove properties other than the required fields).
This would be hard to make a warning for.

The change required to support this in an app would be to either explicitly create a new credentials object (I think this is best to recommend) or using something like _.pick or Ember.Object.getProperties.

[rtablada]

Reviewing git history and issues it looks like this has actually been discussed before in issues as a source of confusion: #170

[rtablada]

To add more issues on to the 🔥 version 3.0.0 has actually different code than master?

https://github.com/jpadilla/ember-simple-auth-token/blob/v3.0.0/addon/authenticators/token.js#L146-L150

[jpadilla]

Hey @rtablada thanks for such a thoughtful description of this issue. Last PR I merged was #173 which was after releasing v3. I'm always down for simplifying API, removing confusion and misdirection. How would you suggest we move forward?

[rtablada]

@jpadilla fully doing this would need to be a major version since it would require a change in code. But I can work up a PR that would be something that could be a minor change with a deprecation warning.

[musaffa]

I was suggesting the same two years ago: #144

[jpadilla]

Closed by #226. Thanks again @rtablada for bringing this up.