Proposal: Remove Use of `getAuthenticateData`
rtablada opened this issue · 6 comments
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.
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.
Reviewing git history and issues it looks like this has actually been discussed before in issues as a source of confusion: #170
To add more issues on to the 🔥 version 3.0.0 has actually different code than master?