-
Notifications
You must be signed in to change notification settings - Fork 45
Fix linkedin #16
base: master
Are you sure you want to change the base?
Fix linkedin #16
Conversation
@danilopolani any updates? |
Ehy, sorry I'm a loot busy in this time due to my work. Anyway I was looking at the code, I remember that to retrieve the profile picture URL was a bit tricky in V2, did you test it? |
Yeah I tried to implemented but it looked complicated and will require extra API calls so I dropped it. |
Ok, but then we can't merge this because we would not guarantee the same output as before. If someone uses the |
@danilopolani it is already there https://github.com/danilopolani/gocialite/pull/16/files#diff-7af7b035f06b4444f6c2e0b4bba1a6a3R31 just empty field |
Yeah, but it can't be an empty field if before it was not, otherwise it's a breaking change |
If the field still exists then it is not a breaking change because it will not panic. It could also be a user without an avatar so it could already be empty. |
Ok, but imagine this flow: My app implements a "Sign in with LinkedIn" feature; when a user clicks on the button and I receive the callback, I always update his avatar to have it up-to-date. Maybe it would not be a "breaking" for the panic itself, but for a regular flow and the data consistency. I hope you got the point from the example above |
Well currently the user does not get an email because of the new restriction linkedin added to the retired API so it is already broken in this sense. |
Upgrade Linkedin API and retrieve email address correctly since V1 disallows email address retrieval.