-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Meteor.loginWithPassword should have a similar signature as Accounts.createUser #13069
Comments
I like your proposal. Feels clear to expect similar and consistent interfaces for functions that are alike. I particularly prefer the approach of passing parameters as objects, which enhances versatility and facilitates quick refactoring if needed. |
Hi @jamauro This is my first open source, can you please guide me? I see that updating the function definition of loginWithPassword in https://github.com/meteor/meteor/blob/devel/packages/meteor/meteor.d.ts with an object would suffice like below function loginWithPassword( I think it should not break any existing functionality, is this the expected fix? |
@nachocodoner any thoughts on whether it should be a breaking change or not? It would certainly be cleaner from a core code standpoint to make a breaking change but would require developers to make a small change in their code. Also I think we'll want Hey @abhinavtps, thanks for your willingness to jump on this one. Before diving into the solution, let's wait and see if the Meteor team thinks it's worth introducing a breaking change or not. The code will look quite a bit different depending on the answer to that question. After we have a clearer picture of what's needed, the bulk of the work will need to happen here:
and here:
The js doc and .d.ts for these functions will need to be updated as well. It will also need tests added / updated here: https://github.com/meteor/meteor/blob/devel/packages/accounts-password/password_tests.js to ensure the new proposed way works as expected. |
I propose avoiding breaking changes by implementing it with polymorphism, supporting both parameter and object approaches. We'll provide a deprecation log for the parameter approach, warning users about the convention change, to be removed in a future release. What are your thoughts? |
I like that approach. @abhinavtps see Nacho’s comment above for the desired solution. If you have follow up questions, feel free to use this thread. 🙂 |
It feels kind of weird that
Accounts.createUser
andMeteor.loginWithPassword
have different signatures. I propose thatMeteor.loginWithPassword
is updated so that you can do this:I suppose it should have backwards compatibility so people aren't forced to rewrite their existing code.
Thoughts?
The text was updated successfully, but these errors were encountered: