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

feat: add more interface for flutter web support #444

Merged
merged 8 commits into from
Nov 17, 2021

Conversation

yuhao900914
Copy link
Contributor

@yuhao900914 yuhao900914 commented Nov 13, 2021

Summary

Expose more interfaces for flutter web support
The following are the API added in this pr.

  1. getDeviceId
  2. getUserId
  3. setMinTimeBetweenSessionsMillis
  4. setEventUploadThreshold
  5. setUseDynamicConfig
  6. setServerZone
  7. setServerUrl

Checklist

  • Does your PR title have the correct title format?
  • Does your PR have a breaking change?:

Copy link
Contributor

@qingzhuozhen qingzhuozhen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR title seems not in semantic format

@@ -1473,6 +1515,7 @@ AmplitudeClient.prototype.logEventWithGroups = function (
groups,
opt_callback,
opt_error_callback,
outOfSession = false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this one in group also used in flutter? Or used in other SDK?

Copy link
Contributor Author

@yuhao900914 yuhao900914 Nov 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flutter doesn't have the logEventWithGroups method. But I think those functions are exposed to Amplitude-Javascript user, so it's better to keep them consistent. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yuhao900914 For other SDKs we have, do we have similaroutOfSession support on logEventWithGroups?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Andorid and iOS are similar, the logEvent function is able to pass groups info and outOfSession together.
- (void)logEvent:(NSString *)eventType withEventProperties:(NSDictionary *)eventProperties withApiProperties:(NSDictionary *)apiProperties withUserProperties:(NSDictionary *)userProperties withGroups:(NSDictionary *)groups withGroupProperties:(NSDictionary *)groupProperties withTimestamp:(NSNumber *)timestamp outOfSession:(BOOL)outOfSession {xxx}

src/amplitude-client.js Outdated Show resolved Hide resolved
src/amplitude-client.js Outdated Show resolved Hide resolved
test/amplitude-client.js Outdated Show resolved Hide resolved
test/amplitude-client.js Outdated Show resolved Hide resolved
test/amplitude-client.js Outdated Show resolved Hide resolved
src/amplitude-client.js Outdated Show resolved Hide resolved
@yuhao900914 yuhao900914 changed the title Flutter web support feat: add more interface for flutter web support Nov 14, 2021
Copy link
Contributor

@jooohhn jooohhn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just some minor changes

  • Should remove library/version code and tests since we already have those features
  • Some typos
  • Tests look like they are failing

Would also be good if you could list APIs in the PR summary for future reference 😄

src/amplitude-client.js Outdated Show resolved Hide resolved
src/amplitude-client.js Outdated Show resolved Hide resolved
src/amplitude-client.js Outdated Show resolved Hide resolved
src/amplitude-client.js Outdated Show resolved Hide resolved
@yuhao900914 yuhao900914 requested a review from jooohhn November 16, 2021 18:49
@yuhao900914 yuhao900914 merged commit 69c18f7 into main Nov 17, 2021
@yuhao900914 yuhao900914 deleted the flutter-web-support branch November 17, 2021 00:34
github-actions bot pushed a commit that referenced this pull request Nov 18, 2021
# [8.12.0](v8.11.1...v8.12.0) (2021-11-18)

### Features

* add more interface for flutter web support ([#444](#444)) ([69c18f7](69c18f7))
* update setLibrary api make it ignore the null value of name or version  ([#449](#449)) ([8e0971e](8e0971e))
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