Changes on how device_id is treated on init

[laziob]

With this changes the consumeUrlParams dont override the client device_id with the passed one. Instead it returns the passed one if exists and the clean queryParams string. Then the amplitude Config is now allowed to recieve a device_id. So then the client creation happens, if a device_id is passed on Config, (which should be the one obtained from consumeUrlParams now), it initiates the client with that device_id, and if not, it initiates with no device_id, so it creates one.

Since this changes modifies some of the base logic of the analytics-client, other components implementation should be revisited to comply with these changes.

Change-type: Major Signed-off-by: Ezequiel Boehler ezequiel@balena.io

[gelbal]

Thanks for the documentation edit Ezequiel. This is good to ship in my opinion. Please remove the draft tag and re-ask for review or comment @balena-ci I self-certify! in the repo to merge this by yourself already.

As I asked in the linked UI PR, feel free to address such a change if you also agree that it helps: Shall we create a convenience method in url-params for this call?

const passedDeviceId = urlParamsHandler.allDeviceIds()[0];

Something like urlParamsHandler.getInitialDeviceId() might convey the logic here better.

[laziob]

@gelbal

Shall we create a convenience method in url-params for this call?

const passedDeviceId = urlParamsHandler.allDeviceIds()[0];

Something like urlParamsHandler.getInitialDeviceId() might convey the logic here better.

mmm I found "initial" to be a somewhat confusing term. We are referring to the first element of the array of devices ids but I dont know if that is exactly a "inital" device_id. Also I cant think of any other use case where someone would need to get that first element other the one we are using, at least not yet.

I do think that writing a convenience method for getting the passedDeviceId could be more clear for the End User though.

Something like: urlParamsHandler.getPassedDeviceId() or urlParamsHandler.getCrossTrackingDeviceId()

What do you think?

[gelbal]

urlParamsHandler.getPassedDeviceId()

I like this @laziob :+1:

[pranasziaukas]

@laziob I think at least one of the CI problems has to do with linting

Error: File src/client.ts hasn't been formatted with prettier
Error: File src/url-params.ts hasn't been formatted with prettier

You could try running npm run prettify to get the styling in place (although I thought the linter runs automatically on commit courtesy of husky).

[gelbal]

@laziob before we merge this, can you please squash your commits?

[laziob]

@gelbal I tried to squash the commits but then it asked me for a merge. Please let me know if the squash worked as intended or if I should somehow squash it again. Thanks!

Also @pranasziaukas I think the ReadMe now its properly indented.

[gelbal]

@laziob it doesn't seem like squash worked. I see 12 commits in the PR now. Let me know if you'd like to do it together.

[laziob]

@gelbal le me try one more time and if I cant make it I'll let you know so we can do it together

[laziob]

@gelbal I somehow got it to 3 commits haha. Still coudnt squashed them all to 1 but let me know if this works

[gelbal]

@laziob we are almost there :smile: Can you please squash all into 1 commit? These 3 commits are undoing each others edits and there is no need to write these back-and-forth changes to our git history.

How do you use git? I use the commandline and I'd do such squash with the command git rebase -i HEAD~3 and interactively edit the commit messages to merge everything.

[gelbal]

On a related note. If the git push is the problem, use force :laughing:

I use this relatively safer force push: git push --force-with-lease

[laziob]

@gelbal Now I think I done it properly!

[laziob]

@gelbal I will now update the UI PR to be in line with this changes. Where can I get the new version for this so as to also update it on that repo?

[pranasziaukas]

Uhm, we haven't changed the Change-type: Major so the versionbot released it as v1.0.0, which is misleading. Shall we rollback and resubmit as a minor change?

[gelbal]

Good point Pranas. Then in a sense this is a major update as we significantly refactored how we initialize the Amplitude client in almost all of the components utilizing analytics-client now. So I'm fine with keeping it 1.0.0.

[laziob]

But isnt it a Major change since it implies to change how the other components use the analytics-client too? Where is the line of what is considered a Major and Minor change?

On Wed, Feb 17, 2021 at 11:50 AM Pranas Ziaukas notifications@github.com wrote:

Uhm, we haven't changed the Change-type: Major so the versionbot released it as v1.0.0, which is misleading. Shall we rollback and resubmit as a minor change?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/balena-io-modules/analytics-client/pull/41#issuecomment-780607458, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFIZHIIF3WVUWIWZIWHZEQTS7PJSTANCNFSM4XE5WV4A .

[gelbal]

Where can I get the new version for this so as to also update it on that repo?

@laziob I don't how long the lag would be. I hope by the time you update the UI PR, you'll be able to retrieve the new version. So please update package.json entry for analytics-client to point to 1.0.0 or higher. Let's see if npm would be able to retrieve it fine already.

[pranasziaukas]

Not urgent, but I'd suggest adding at least some unit tests alongside new methods in the future.