Added a reinitialize method to Fides.js
#4812
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Passing run #7368 ↗︎
Details:
Review all test suite changes for PR #4812 ↗︎ |
||||||||||||||||||||
| @@ -172,7 +172,7 @@ const init = async (config: FidesConfig) => { | |||
| // Initialize the CMP API early so that listeners are established | |||
| initializeTcfCmpApi(); | |||
| if (initialFides) { | |||
| Object.assign(_Fides, initialFides); | |||
| Object.assign(this, initialFides); | |||
There was a problem hiding this comment.
Explain why you think this is preferable - and are we sure it's safe to refactor like this?
There was a problem hiding this comment.
This is preferable because it's closer to functional style which is preferable because it makes code easier to reason about and maintain. By functional I mean functions that take all the data they operate on as arguments and just return values, without modifying any external state (including the arguments). Pure idempotent functions are the best, which init() certainly isn't, but it's a matter of degree. In JS as well as C++ and any other OOP language I know, this is just another argument to the function so Fides.init() is syntax sugar for init(Fides).
It also makes it easier to unit test when you're not relying on some captured state. We could import the init function in a test file and call it repeatedly on different objects to test its behavior.
As for safety, I think that's a very relevant question. If you're working with global state, you have to consider any other code that might be using and changing it. The more visible some state is, the more code you have to audit, with the global state being the most visible of all. Luckily, _Fides is only visible to this module so it's not too bad. The only possible unsafety I can think of with this change is if someone is doing something like this:
const { init } = Fides;
init();But they really shouldn't be doing that in the first place 😄
guncha
changed the title
reinitialize method to Fides.js
guncha
changed the title
reinitialize method to Fides.jsreinitialize method to Fides.js
clients/fides-js/src/fides.ts
Outdated
| if (!this.prevConfig || !this.initialized) { | ||
| throw new Error("Fides must be initialized before reinitializing"); | ||
| } | ||
| return this.init(this.prevConfig); |
There was a problem hiding this comment.
I know this is maybe really Hard™️, but I think we need to ensure that Fides.reinitialize cleans up our existing rendered DOM elements automaticaly... 😄
I wrote some quick Cypress specs to exercise this and toggled fides_embed off and on. Since reinitialize doesn't wipe previously rendered elements, I could get into a fun state where I had both an embedded and overlay banner running simultaneously:
There was a problem hiding this comment.
What do you mean, that's a FEATURE!
There was a problem hiding this comment.
OK, I added some code to initOverlay to unmount previously rendered instances, which makes this work swimmingly.
|
OK @guncha, this is ready for your re-review. I think it looks pretty good... |
Closes https://ethyca.atlassian.net/browse/PROD-1967
Description Of Changes
Adds a new
reinit()method to FideJS global as well as aprevConfigproperty that stores the configuration that was used to callinit. There are also minor changes to theinitfunctions to treat them like instance methods, which they actually are, instead of programming with globals.Opening this now to get some early feedback. This is being used in my WT branch to "move" the embed to the slideout and back. It works fine so I don't expect any big changes though perhaps we want to consider supporting multiple embeds per page (this already works basically, but would need to be more carefully considered). We can discuss in the forthcoming PR.
Pre-Merge Checklist
CHANGELOG.md