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

Upgrade to ldclient-js 1.7.4 #28

Merged
merged 1 commit into from
Aug 15, 2019
Merged

Upgrade to ldclient-js 1.7.4 #28

merged 1 commit into from
Aug 15, 2019

Conversation

patcoll
Copy link
Collaborator

@patcoll patcoll commented Mar 1, 2019

Hey @achambers, as a first step I upgraded the ldclient-js library to version 1.7.4, which is the one right before 2.0.0. I did this to bisect the rather large jump in version numbers, so that when the upgrade to 2.0.0 happens, we can rule out problems from any changes in the 1.x series.

All tests pass. (Could we set up TravisCI or CircleCI for open source purposes to see CI results on GitHub?)

I don't have quick access to test this in a prod or even a test environment. Is it possible for you to try it out and see if there are any red flags?

@patcoll patcoll requested a review from achambers March 1, 2019 20:06
@achambers
Copy link
Collaborator

@patcoll How'd you get on with the new LD account?

@patcoll
Copy link
Collaborator Author

patcoll commented Apr 1, 2019

@achambers I haven't had the chance to loop back around to it, unfortunately.

@achambers
Copy link
Collaborator

@patcoll Mate, sorry about the lack of comms. Been crazy busy. I'm going to switch a little bit of focus to this today so hopefully we can get this thing shipped :)

@achambers achambers force-pushed the upgrade-ldclient-js-174 branch from ddc85d9 to 8cd8654 Compare August 13, 2019 12:19
@achambers
Copy link
Collaborator

achambers commented Aug 15, 2019

Just looking at the changelog for the changes. The following are things of interest:

1.1.13

  • Emits error event now when error connecting. This is potentially something we should use. In the last two apps I've built I've needed to set the local client in the case of an error so that we default to the local flags. Might be useful if the addon does this for us.

1.3.0

  • waitUntilReady seems interesting and also identify is promise aware. Maybe we can remove the callbacks and use async/await or promises

1.6.2

  • We don't implement this track property. Maybe it's something to leave until someone requests it though.

Ok, so, maybe the error event is worth looking at as a part of this PR. Or maybe we just leave it, upgrade, and add this stuff as improvements later.

Thoughts?

@achambers achambers force-pushed the upgrade-ldclient-js-174 branch from 8cd8654 to 23f2a7c Compare August 15, 2019 09:31
Copy link
Collaborator

@achambers achambers left a comment

Choose a reason for hiding this comment

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

Have tested this against my work app and it works fine. Let's merge it 👍

@achambers achambers merged commit 67d5399 into master Aug 15, 2019
@achambers achambers deleted the upgrade-ldclient-js-174 branch August 15, 2019 09:53
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.

2 participants