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.

Sign up for GitHub

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: update setLibrary api make it ignore the null value of name or version #449

Merged
merged 12 commits into from
Nov 18, 2021
Merged

feat: update setLibrary api make it ignore the null value of name or version #449

Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
772cac6
feat: add more interface for flutter web support
yuhao900914 Nov 13, 2021
0824fdd
patch: code clean up and other fix
yuhao900914 Nov 13, 2021
60357ae
test: add setUserId test
yuhao900914 Nov 13, 2021
579b3be
patch: test update
yuhao900914 Nov 14, 2021
9632966
patch: remove unnecessary api interface
yuhao900914 Nov 15, 2021
fc4d624
patch: make setLibrary function able to seprate set library name and …
yuhao900914 Nov 16, 2021
05245f9
patch: make setLibrary function able to seprate set library name and …
yuhao900914 Nov 16, 2021
fb50d79
fix: merge conflict
yuhao900914 Nov 16, 2021
fda6870
patch: setLibrary improvement
yuhao900914 Nov 17, 2021
55dc415
patch: merge conflict
yuhao900914 Nov 17, 2021
aa3b2df
patch: make groupIdentify and identify api enable to pass outOfSessio…
yuhao900914 Nov 17, 2021
f7febab
test: add test and chagnes based on comments
yuhao900914 Nov 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions src/amplitude-client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1114,7 +1114,7 @@ var _convertProxyObjectToRealObject = function _convertProxyObjectToRealObject(i
* var identify = new amplitude.Identify().set('colors', ['rose', 'gold']).add('karma', 1).setOnce('sign_up_date', '2016-03-31');
* amplitude.identify(identify);
*/
AmplitudeClient.prototype.identify = function (identify_obj, opt_callback, opt_error_callback) {
AmplitudeClient.prototype.identify = function (identify_obj, opt_callback, opt_error_callback, outOfSession) {
Copy link
Contributor

Choose a reason for hiding this comment

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

May be good to add unit test for out of session funcitonality

if (this._shouldDeferCall()) {
return this._q.push(['identify'].concat(Array.prototype.slice.call(arguments, 0)));
}
Expand Down Expand Up @@ -1142,6 +1142,7 @@ AmplitudeClient.prototype.identify = function (identify_obj, opt_callback, opt_e
null,
opt_callback,
opt_error_callback,
outOfSession,
);
} else {
_logErrorsWithCallbacks(opt_callback, opt_error_callback, 0, 'No request sent', {
Expand All @@ -1162,6 +1163,7 @@ AmplitudeClient.prototype.groupIdentify = function (
identify_obj,
opt_callback,
opt_error_callback,
outOfSession,
) {
if (this._shouldDeferCall()) {
return this._q.push(['groupIdentify'].concat(Array.prototype.slice.call(arguments, 0)));
Expand Down Expand Up @@ -1205,6 +1207,7 @@ AmplitudeClient.prototype.groupIdentify = function (
null,
opt_callback,
opt_error_callback,
outOfSession,
);
} else {
_logErrorsWithCallbacks(opt_callback, opt_error_callback, 0, 'No request sent', {
Expand Down Expand Up @@ -1908,11 +1911,9 @@ AmplitudeClient.prototype.__VERSION__ = function getVersion() {
* @param {string} name - Custom library name
* @param {string} version - Custom library version
*/
AmplitudeClient.prototype.setLibrary = function setLibrary(
name = this.options.libraryName,
version = this.options.libraryVersion,
) {
this.options.library = { name: name, version: version };
AmplitudeClient.prototype.setLibrary = function setLibrary(name, version) {
if (name) this.options.library.name = name;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe wrap other than put them in the same line

if (version) this.options.library.version = version;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should do an explicit check for null or undefined. Edge case conditions for falsey values may lead to unexpected outcomes (ike 0 or '' )

};

/**
Expand Down