feat: utilize generics in UserPreferencesService

[danielsiemens]

Summary

First off, a big BIG thank you to @gmarques-mParticle for his assistance in making this properly generic. You're the man!

• Replaces UserPreference with UserPreferencePerScope
• Thanks to GM, this is now generic by user preference key. This allows different preferences to specify different data structures. This change should not break existing uses in CDP/Analytics, but we will write new unit tests on those platforms to confirm
• "Why not use localStorage like recently-viewed in nancy?" -> Because we know that we want to add a backend to UPS in 2025, and it makes sense that beyond just a simple toggle we may want to store more complex data types as well. Replacing the optedIn attribute outright would break existing usage patterns, and we don't want users forced to re-opt in!
• Updates unit tests

Testing Plan

• Was this tested locally? If not, explain why.
• {explain how this has been tested, and what, if any, additional testing should be done}

+80% coverage on user-preference related files

Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)

• Closes https://mparticle-eng.atlassian.net/browse/UNI-1253

[danielsiemens]

Added so we can check unit test coverage with vitest. This is a dev dependency

[github-actions]

PR Preview Action v1.4.8
Preview removed because the pull request was closed.
2024-11-28 21:23 UTC

[tibuurcio]

Summary already goes a bit into the choices made here. From our chats I know you've tried a generics approach before this solution @danielsiemens, do you mind going over that context and some of the challenges you faced? Documenting it for posterity.

[tibuurcio]

Would also be interesting to state some of the alternatives to the current approach we discussed sync yesterday. Just cause there's a lot of ways to accomplish what we want here and besides the localStorage approach the PR doesn't convey the amount of thought and tradeoff analysis we put into it 😅

If possible I'd state a few options and highlight the one we decided to go with!

[tibuurcio]

Now that I'm thinking of it, will this update require changes in tha platforms implementations once it reaches a new Aquarium version? If yes, then that's technically a breaking change 🥲

This is also a sensitive part of the application, I know we have tests here but usage in the platforms is a bit different. Can we have a feature version released for this and try a POC of it in the platforms just to see how it goes?

We don't usually do proper semver breaking changes in Aquarium currently. Maybe too extreme but would it be worth to duplicate the implementation? Probably not, but just raising the idea.

Either way nice work @danielsiemens and @gmarques-mParticle

[gmarques-mParticle]

Now that I'm thinking of it, will this update require changes in tha platforms implementations once it reaches a new Aquarium version? If yes, then that's technically a breaking change 🥲

This is also a sensitive part of the application, I know we have tests here but usage in the platforms is a bit different. Can we have a feature version released for this and try a POC of it in the platforms just to see how it goes?

We don't usually do proper semver breaking changes in Aquarium currently. Maybe too extreme but would it be worth to duplicate the implementation? Probably not, but just raising the idea.

Either way nice work @danielsiemens and @gmarques-mParticle

I like the idea to duplicate the implementation, so we can have a FF on CDP to validate it in the consumer side

[tibuurcio]

Now that I'm thinking of it, will this update require changes in tha platforms implementations once it reaches a new Aquarium version? If yes, then that's technically a breaking change 🥲
This is also a sensitive part of the application, I know we have tests here but usage in the platforms is a bit different. Can we have a feature version released for this and try a POC of it in the platforms just to see how it goes?
We don't usually do proper semver breaking changes in Aquarium currently. Maybe too extreme but would it be worth to duplicate the implementation? Probably not, but just raising the idea.
Either way nice work @danielsiemens and @gmarques-mParticle

I like the idea to duplicate the implementation, so we can have a FF on CDP to validate it in the consumer side

hey @gmarques-mParticle, both @danielsiemens and I validated the changes in the platforms locally. Discussion is in the slack thread in PR canvas. I'd like your input again here. We have a separate PR in Nancy to perform the changes to the UPS implementation, so IMO with that plus the unit tests that Dan implemented in Nancy, as well as the Test Plan in the PR, we should be good for keeping only a single implementation.

The important part that worried me is we were able to verify backwards compatibility is maintained between the implementations, both by unit and manual testing.

Given that I'd suggest we do not duplicate...

[tibuurcio]

This has been tested both in Aquarium but also in the platforms and we are good to go. This will also be thouroughfully tested in the platform PR introducing it.

[danielsiemens]

Maintaining two implementations would have a much higher likelihood of causing bugs when we can verify through unit tests that the legacy cookies are still valid and working. I agree with @tibuurcio that we should not have a duplicate implementation.
cc @gmarques-mParticle

[mparticle-automation]

🎉 This PR is included in version 1.36.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀