-
Notifications
You must be signed in to change notification settings - Fork 29
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
GH-62: adding jwt-enabled API keys #67
base: master
Are you sure you want to change the base?
Conversation
Neat - thanks! |
@@ -27,8 +27,14 @@ class IterableFlutter { | |||
_channel.setMethodCallHandler(nativeMethodCallHandler); | |||
} | |||
|
|||
static Future<void> setEmail(String email) async { | |||
await _channel.invokeMethod('setEmail', email); | |||
static Future<void> setEmail(String email, String jwt) async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make jwt nullable in order to communicate that it isn't strictly required. Some implementations either don't care about having a JWT, or just want to get something quick put together to ensure the package works.
@@ -27,8 +27,14 @@ class IterableFlutter { | |||
_channel.setMethodCallHandler(nativeMethodCallHandler); | |||
} | |||
|
|||
static Future<void> setEmail(String email) async { | |||
await _channel.invokeMethod('setEmail', email); | |||
static Future<void> setEmail(String email, String jwt) async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No support for setUserId? Seems easy enough to add.
android/build.gradle
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think PRs should be scoped specifically to what they're trying to do - support JWT in this case. Is is necessary to change this in order to do that? Maybe consider creating another PR for whatever you need these changes for.
Imagine if something in these new versions broke and needed to revert this PR - now we lose JWTs! Or have to spend extra time cherrypicking exactly what we need to revert.
if (activeLogDebug) { | ||
configBuilder.setLogLevel(Log.DEBUG) | ||
configBuilder.setLogLevel(Log.VERBOSE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably not necessary to include in the PR and probably makes sense to change locally when you need to check logs for something.
Folks, sorry to be the messenger of bad news, but as organization owner of this package, La Haus is no longer maintaining this repo, then I'd suggest you forking it and work on your own solution. |
This PR aims to take advantage of the optional JWT 2nd parameter when calling
IterableFlutter.setEmail
. I'm not sure what PR etiquette looks like for your team, but I'd love any feedback you have on how I can improve the PR so we can get it merged & others can take advantage of this feature.Credit @QoLTech for his original inspiration for this PR.