Use getCmp - #3925
Use getCmp#3925oliverlloyd wants to merge 13 commits into
getCmp#3925Conversation
|
Size Change: -15.2 kB (-1%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
| @@ -1,94 +0,0 @@ | |||
| import { hasRequiredConsents } from './hasRequiredConsents'; | |||
There was a problem hiding this comment.
Definitely happy for these tests to go if they're not providing value. I can see that they've got some shortcomings and use some patterns I don't love, such as jest mocking at the module level, and they're also leaking some internal implementation details which arguably this test shouldn't care about (disclaimer: I'm pretty sure I wrote these tests!). Maybe there's a way of reworking the tests (or the implementation) to move away from some of these patterns? Would be very happy to pair on this if you're interested!
There was a problem hiding this comment.
Yes, please. I actually have this task as a todo on this PR (but only in my head/some text document on my laptop - I could probably surface this more!)
I was hoping that there would be a way to implement a Cypress test that verified the UI responded as expected based on different consent settings.
| return window.guCmpHotFix; | ||
| }; | ||
|
|
||
| export const guCmp = getCmp(); |
There was a problem hiding this comment.
I think maybe I've just not drunk enough coffee yet this morning but I don't think I've seen an export which exports the result of calling a function before. Can you explain when that getCmp() call will happen? Are these static import/exports not resolved when we're building/compiling the JS asset to ship to the client? Sorry if I'm missing something obvious!
There was a problem hiding this comment.
lol, not sure how I ended up with that pattern! Fixed now.
There was a problem hiding this comment.
In the end I rationalised the approach here to something a bit more semantic; I'm now using getCmp and executing it in the files each time.
|
this feels very brittle to me – it is 100% not intended to be the public interface (unless this has changed @shtukas @kenoir?)
why is this? there should always be only one instance of the code available to DCR? what's the overhead?
as mentioned above, there is but it's undocumented and not intended for external use |
Fair question. And I'm actually super happy for this approach to be challenged in general. The motivation for this work was rooted in the cmp lib not being server safe so when it's an import that gets evaluated and throws errors whenever we server side render components. We've been getting away with this up to now by using Portals and dynamic imports but with the transition to islands we started to hit this problem much more often (the island version of a Portal evaluates the component on the server). So, having this window prop solves this in a nice, low effort way but maybe the larger solution is to make cmp server safe? |
|
I'm not sure how meaningful it is to have a 'server safe' cmp (since all the meaningful state is clientside). However I wonder if this should be internal to the CMP. As in consumers shouldn't have to worry about getting the instance, or making sure it's the instance it should take care of it itself? I echo Alex's concern about increasingly relying on something called 'hotfix' but I think the solution is to rename it, formally make the CMP library use it internally, and have it check itself internally on any method call, transparent to rh consumer. And the consumer just calls things blithely, safe in the knowledge that the CMP internally ensures it's refering to a single instance of its state. I can do a quick mockup of what I'm talking about of I'm not being clear here (my code is sometimes more articulate than I am). |
|
Closing now that we have #4126 🎉 |
What does this change?
Implement the
getCmpfunction as a replacement for importing thecmpobject each time.Why?
We don't want to repeatedly import the cmp code from the npm lib and there's already a copy of it set on the window object for us to access so we're leaning into this pattern.