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

Move to new ld client package, import package via ember-auto import #133

Merged
merged 8 commits into from
Sep 5, 2020

Conversation

chrisvdp
Copy link
Contributor

@chrisvdp chrisvdp commented Mar 2, 2020

Hello, thanks for doing the leg work to get this repo setup!

We required the latest launch-darkly package for our internal product, so I thought I would share the changes I made via PR.

If this gets merged, there are some subsequent changes to the code that allows for anonymous users that I can push.

cc: @mansona

@ToMoCoop
Copy link

Code looks good to me - can we get this merged @achambers or whoever else is it may concern?

@achambers
Copy link
Collaborator

Thanks for this @chrisvdp .

I'll be casting some focus towards this this week. There are few things I need to verify here before merging due to how far behind we are with the client lib version.

Between @mansona and myself, hopefully we can get some traction on this by the end of the week.

/cc @ToMoCoop

@chrisvdp
Copy link
Contributor Author

@achambers sounds good. Let me know if there are any tasks I can take on to help.

@RobbieTheWagner
Copy link
Member

@mansona @achambers anything I can do to help get these LD client update PRs in?

@chrisvdp
Copy link
Contributor Author

@achambers any updates?

@jaredgalanis
Copy link
Member

@achambers @mansona, just checking back in here. Anything I can do to help with this PR? I've got some time to devote to the project.

.yarnrc.yml Outdated Show resolved Hide resolved
mansona
mansona previously requested changes Sep 3, 2020
Copy link
Collaborator

@mansona mansona left a comment

Choose a reason for hiding this comment

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

all in all I'm in favour of this change, bring on the future 🎉

I have 2 suggestions now that we have changed how this works 👍 one of which is mostly a style thing ;) but there are some benefits to it 😂

addon/services/launch-darkly-client-remote.js Outdated Show resolved Hide resolved
addon/services/launch-darkly-client-remote.js Outdated Show resolved Hide resolved
@achambers
Copy link
Collaborator

Firstly, massive apologies for being so crappy with getting to this.

Has anyone been through the release notes to see what has changed that we might need to be aware of? I did this quite some time ago and there were a handful of things that felt like should be considered before/as a part of upgrading.

Just looking through them again now and these things feel like we need to, at the very least, consider whether there is something we need to take in to account...

Releases since 1.7.4 HERE

2.0.0

  • The new configuration option sendEventsOnlyForVariation, if set to true, causes analytics events for feature flags to be sent only when you call variation. Otherwise, the default behavior is to also send events when you call allFlags, and whenever a changed flag value is detected in streaming mode.

The fact that allFlags sent analytics events (the LD interface would say that the flags had been requested "x seconds ago" as a result of calling allFlags which was inaccurate) meant I had to build a work around in to the code. Might be nice to not have to do that now seeing as this new option exists. My gut says, not critical for this PR. But maybe something to follow up on if someone is interested in contributing to this repo.

2.7.0

New client method waitForInitialization returns a Promise, like waitUntilReady; but while waitUntilReady will be resolved as soon as client initialization either succeeds or fails, waitForInitialization will be resolved only if initialization succeeds, and will be rejected (with an error object) if it fails.

This repo currently waits 10 seconds for the "ready" event and rejects if there is an issue. Feels like we should move to using this waitFortInitialization function. Feels like an improvement and valuable thing for this PR

2.8.0

The use of a streaming connection to LaunchDarkly for receiving live updates can now be controlled with the new client.setStreaming() method, or the equivalent boolean streaming property in the client configuration. If you set this to false, the client will not open a streaming connection even if you subscribe to change events (you might want to do this if, for instance, you just want to be notified when the client gets new flag values due to having switched users). If you set it to true, the client will open a streaming connection regardless of whether you subscribe to change events or not (the flag values will simply be updated in the background). If you don't set it either way then the default behavior still applies, i.e. the client opens a streaming connection if and only if you subscribe to change events.

Do we need to understand and consider this? This repo supports streaming. I'm not fully clear on this change and am unsure if we need to consider it. Decide/understand whether this is relevant to this PR

Other thoughts

  • Functions that return callbacks, also return promises if no cb is provided. Do we want to consider switching to promises, or even async/await?

Ok, so there's not as much as I first thought. Still, what are people's thoughts on these things and whether or not we need to consider them as a part of this PR?

/cc @mansona

@jaredgalanis
Copy link
Member

Firstly, massive apologies for being so crappy with getting to this.

Has anyone been through the release notes to see what has changed that we might need to be aware of? I did this quite some time ago and there were a handful of things that felt like should be considered before/as a part of upgrading.

Just looking through them again now and these things feel like we need to, at the very least, consider whether there is something we need to take in to account...

Releases since 1.7.4 HERE

2.0.0

  • The new configuration option sendEventsOnlyForVariation, if set to true, causes analytics events for feature flags to be sent only when you call variation. Otherwise, the default behavior is to also send events when you call allFlags, and whenever a changed flag value is detected in streaming mode.

The fact that allFlags sent analytics events (the LD interface would say that the flags had been requested "x seconds ago" as a result of calling allFlags which was inaccurate) meant I had to build a work around in to the code. Might be nice to not have to do that now seeing as this new option exists. My gut says, not critical for this PR. But maybe something to follow up on if someone is interested in contributing to this repo.

2.7.0

New client method waitForInitialization returns a Promise, like waitUntilReady; but while waitUntilReady will be resolved as soon as client initialization either succeeds or fails, waitForInitialization will be resolved only if initialization succeeds, and will be rejected (with an error object) if it fails.

This repo currently waits 10 seconds for the "ready" event and rejects if there is an issue. Feels like we should move to using this waitFortInitialization function. Feels like an improvement and valuable thing for this PR

2.8.0

The use of a streaming connection to LaunchDarkly for receiving live updates can now be controlled with the new client.setStreaming() method, or the equivalent boolean streaming property in the client configuration. If you set this to false, the client will not open a streaming connection even if you subscribe to change events (you might want to do this if, for instance, you just want to be notified when the client gets new flag values due to having switched users). If you set it to true, the client will open a streaming connection regardless of whether you subscribe to change events or not (the flag values will simply be updated in the background). If you don't set it either way then the default behavior still applies, i.e. the client opens a streaming connection if and only if you subscribe to change events.

Do we need to understand and consider this? This repo supports streaming. I'm not fully clear on this change and am unsure if we need to consider it. Decide/understand whether this is relevant to this PR

Other thoughts

  • Functions that return callbacks, also return promises if no cb is provided. Do we want to consider switching to promises, or even async/await?

Ok, so there's not as much as I first thought. Still, what are people's thoughts on these things and whether or not we need to consider them as a part of this PR?

/cc @mansona

I don't see any blockers in the updates identified in these release notes. I'd personally prefer to address any changes that may be desirable as a result of these updates in follow up PRs. I'm happy to take on the work of making any changes that are desired or necessary. I am hopeful that the forward momentum we'll gain by landing this PR will be a catalyst for fully updating the addon in the near future.

Happy to reconsider if someone sees something that is problematic enough to need addressing now.

This was referenced Sep 4, 2020
Chris van der Ploeg and others added 7 commits September 5, 2020 09:57
@jaredgalanis jaredgalanis merged commit 8f5563b into adopted-ember-addons:master Sep 5, 2020
@chrisvdp chrisvdp deleted the update-ld-client branch September 10, 2020 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants