feat: add removeContextField method

zubko commented 6 months ago

About the changes

Adding a removeContextField, because setContextField accepts only string and currently removing a field means manipulation with getContext()'s result to turn it to IMutableContext first or the console warn will be thrown.

Also setContextField was already lagging behind the updateContext in how it fetches the data after an update. Someone changed updateContext but not setContextField, so I moved that code to updateToggles call to be able to reuse it. Lmk what you think.

Important files

index.ts
index.test.ts

Discussion points

extra method vs making setContextField accepting undefined as a value to remove it
moving the re-fetch logic to another method, what should be a name of it - updateToggles refetchToggles .. ?
also setContextField was not waiting for the end of the update while updateToggles does. Should setContextField be async as well?

zubko commented 6 months ago

Taking it out of draft if it helps to bring some attention to the PR. There are some points to discuss, but in general it's ready for review.

sighphyre commented 5 months ago

Hey @zubko,

The new method looks great, I think that's pretty sane to include. Would you be up for updating the README with the details so that other folks know it exists?

Just to address your discussion points:

extra method vs making setContextField accepting undefined as a value to remove it

I like your approach. A new method feels safer and clearer than giving the existing methods a new responsibility

moving the re-fetch logic to another method, what should be a name of it - updateToggles refetchToggles .. ?

I don't see an issue with the name you've chosen here

also setContextField was not waiting for the end of the update while updateToggles does. Should setContextField be async as well?

This one might be a problem. I suspect that wasn't being awaited for a good reason. Making this async now means updating the context is now a blocking call. Can I chat with my colleagues about the impact here and get back to you?

sighphyre commented 5 months ago

Hey @zubko! Okay, so I chatted with the rest of the team about this:

also setContextField was not waiting for the end of the update while updateToggles does. Should setContextField be async as well?

Making this properly async is probably a problem here. This spins off effectively a background task to lazily update toggles in the background. That definitely leads to a more consistent experience but it does mean that updating context becomes blocking on a network request, which we think is probably a big and aggressive change. How would you feel about leave that synchronous for this PR?

zubko commented 5 months ago

Hi @sighphyre

Thanks for the answers and checking the setContextField async vs no-async declaration

How would you feel about leave that synchronous for this PR?

Ok. I'll make a fix. I'd assume then that removeContextField should mirror the API of set..? So then I'd also make it non returning a Promise.

Would you be up for updating the README with the details so that other folks know it exists?

Sure. Shall I bump a minor version then as well?

TLDR on the async vs non-async, from my perspective

My only concern that making setContextField async is an API change, and API change means major version change in semver, if we play 100% by the rules. Previously this function returned undefined and now it will return Promise. API change.

Other than that, making it async is not that a big change as it may look like. Because the fetch is at the bottom and all other operations are sync, so making it async equals to return a promise from it. This is basically equivalent:

async function Func(): Promise<void> {
  .. some sync code
  await someFetch();
}

and

function Func(): Promise<void> {
  .. some sync code
  return someFetch();
}

(There is even some linter rule to prevent using async for the functions when it's just 1 await operation at the end)

The update is 99.9% safe, because if setContextField becomes async, but no-one awaits for it, that's basically the same as if it was not async. And if wasn't async before then probably no-one awaits for it in the existing code, so their code won't break with the upgrade. :)

But JavaScript does allow to put await before functions which do not return Promise, so theoretically someone could have put await in front of it.

For 100% backwards compatibility it's better to leave it alone then and to not change the API.

sighphyre commented 5 months ago

Ok. I'll make a fix. I'd assume then that removeContextField should mirror the API of set..? So then I'd also make it non returning a Promise.

Sounds good!

Sure. Shall I bump a minor version then as well?

Would you mind? I appreciate you so much!

TLDR on the async vs non-async, from my perspective

From a technical perspective I agree with you completely. We do try to stick to semver on this project though. It should be safe, I'm sure it'll be 99.9% okay. But if I've learned anything from maintaining a largeish OSS repo, that 0.1% of humans do the strangest things and create a lot of noise and I try to be safe rather than sorry these days.

kwasniew commented 5 months ago

@zubko This change has been applied in https://github.com/Unleash/unleash-proxy-client-js/pull/196 and released in 3.4.0-beta.0

zubko commented 5 months ago

🙌🏼 Thanks a lot @kwasniew

.. I was failing to find the time to finalize this one - I'm super happy you resolved it 👏🏼

Unleash / unleash-proxy-client-js