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.

Sign up for GitHub

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

Jump to bottom

JS API setUserId() ignores numeric ids #10421

Closed
kernins opened this issue Aug 22, 2016 · 3 comments
Closed

JS API setUserId() ignores numeric ids #10421

kernins opened this issue Aug 22, 2016 · 3 comments
Labels
answered For when a question was asked and we referred to forum or answered it.

Comments

@kernins
Copy link

kernins commented Aug 22, 2016

setUserId:function(cA){if(!A(cA)||!cA.length){return} ...

If you're doing arg validation, then do it properly and check for all possibly acceptable types.
Ints should definitely be acceptable here.

And it is extremely stupid idea to silently return on invalid argument. Is it really that hard to throw a TypeError?
@mattab
Copy link
Member

mattab commented Aug 22, 2016

Is it really that hard to throw a TypeError?

Really not that hard. Want to submit a pull request maybe?

@kernins
Copy link
Author

kernins commented Aug 22, 2016

For now it may break things for some lazy users. By tolerating invalid argument values current behavior implicitly allows unconditional setUserID() invokations (I mean calling it whether or not visitor is authenticated and a valid userID is available).

Personally I see no problem in breaking this by throwing an exception on invalid userID - lazy programmers must suffer, it helps them to stop being lazy) But this is not my project to decide.

There is also related issue #7556 on resetting user id, which would be especially usefull for single-page apps. Also see my comment there.

If you're OK with my PoV, I may do this in forseeable future, when I have some time

@mattab
Copy link
Member

mattab commented Nov 11, 2016

If you're OK with my PoV, I may do this in forseeable future, when I have some time

Sounds good, at least fixing the type checking should be easy. looking forward to your PR 👍

@mattab mattab closed this as completed Nov 11, 2016
@mattab mattab added the answered For when a question was asked and we referred to forum or answered it. label Nov 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
answered For when a question was asked and we referred to forum or answered it.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@mattab @kernins