ContextProviderExtension: use spread operator instead of clonedeep

[sahil143]

Fixes: https://issues.redhat.com/browse/ODC-4430

Analysis: The guided tour was not working because of some issue with ContextProvider extension.

Solution: using spread operator instead of lodash cloneDeep while sanitizing extension seems to fix it.

Browser:

• Chrome

[sahil143]

/cc @rohitkrai03 @vojtechszocs

[sahil143]

/kind bug

[glekner]

I believe you should create a BZ for this

[andrewballantyne]

I believe you should create a BZ for this

Until the repo is setup to block without bugzillas, there is no requirement to do that unless you're backporting.

[andrewballantyne]

The premise of this change makes me uncomfortable. Something is "hoping" for the mutation in order to work. That's a bigger issue we should look to fix at some point (doesn't need to block this fix).

cc @vojtechszocs

[vojtechszocs]

@andrewballantyne @glekner @sahil143

ContextProvider extensions directly reference React Context.Provider component via their Provider property.

According to docs, _.cloneDeep clones arrays, array buffers, booleans, date objects, maps, numbers, Object objects, regexes, sets, strings, symbols, and typed arrays.

Debugging shows that Context.Provider component references the Context object. Since _.cloneDeep works recursively, this seems to break the link between Context.Provider and the original Context object, which explains why it's not updated at runtime.

TL;DR we can use _.clone instead of _.cloneDeep or keep using object spread { ...e }.

[andrewballantyne]

Interesting 🤔 -- both shallow spreads/clones and deep clones change the object reference ... but something nested within the data is being referenced instead. Object mutations are tricky beasts, as long as we don't get into a practice of doing it, we can take some assumptions here.

I find it odd that we freeze the object (MDN docs), which I'm finding out doesn't actually prevent mutations of nested values 🤔 , so it's mutable after the fact too?

const obj = {
  prop: {
    value: {
      item: 42
    }
  }
};

Object.freeze(obj);

try {
obj.prop.value.item = 33;
// Throws an error in strict mode
} catch(e) {}

console.log(obj.prop.value.item);
// output: 33

[andrewballantyne]

Ah doh! I guess I assumed it would be recursive, but it makes sense that it is not. It's freezing the top level object and it's direct prosperities but each property that is an object is a new object that needs to be frozen.

EDIT: which is explained here if I read the whole MDN page lol

[vojtechszocs]

@andrewballantyne

Object mutations are tricky beasts, as long as we don't get into a practice of doing it, we can take some assumptions here.

Right, we currently assume that static plugins won't modify extension objects in any way within their plugin.tsx module.

I find it odd that we freeze the object (MDN docs), which I'm finding out doesn't actually prevent mutations of nested values , so it's mutable after the fact too?

Yes, Object.freeze affects only the properties declared directly on that object. So we're basically enforcing that extension's type, properties and flags (plus computed ones pluginName and uid) cannot be changed.

Maybe we should do a deep freeze instead of just the regular one. That way, even if a plugin tries to modify some nested property like e.properties.foo it would stay the same. We can do this as part of #6101.

[andrewballantyne]

Yeah, wonder if we should just fix the mutations within this file @vojtechszocs for a little QoL... we don't need to do it part of this PR though.

https://github.com/openshift/console/pull/6243/files/219277593bbf377d8427971251d8101a43eb0af9#diff-7aa93e478ecd93277b784e0c7ea91f8cR5-R20

Replace with spreads against the original object... it's still a mutation, but it's a smaller scoped one? Again, not sure if this is anything more than academic heh.

[invincibleJai]

I can see guided tour in developer perspective with this fix.

[vojtechszocs]

/lgtm

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[andrewballantyne]

/retest

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sahil143, vojtechszocs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• frontend/packages/console-plugin-sdk/OWNERS [vojtechszocs]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment