Conversation
|
Neat - thanks! |
|
|
||
| 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.
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.
|
|
||
| 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.
No support for setUserId? Seems easy enough to add.
There was a problem hiding this comment.
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.
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. |
3 participants
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.