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.

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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Conversation

kdmny
Copy link

@kdmny kdmny commented Sep 4, 2023

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.

@QoLTech
Copy link

QoLTech commented Sep 4, 2023

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 {
Copy link

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 {
Copy link

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.

Copy link

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)
Copy link

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.

@cdmunoz
Copy link
Contributor

cdmunoz commented Sep 7, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants