Clone global data when bootstrapping datastores.#12824
Conversation
techanvil
force-pushed
the
enhancement/5798-add-global-data-helper
branch
from
fc16153 to
6f29e30
Compare
🎭 Playwright reports for 8650016: 📚 Storybook for 8650016: 📦 Build files for 8650016:
|
|
Size Change: 0 B Total Size: 2.92 MB ℹ️ View Unchanged
|
techanvil
changed the title
aaemnnosttv
left a comment
aaemnnosttv
left a comment
There was a problem hiding this comment.
Thanks @techanvil – this LGTM, just a few suggestions worth considering.
I'm also curious if the underlying issue is still relevant for stores that use the Immer-based createReducer – I seem to recall that this might be doing this kind of dereferencing already anyways but you also worked on that and might know better off the top of your head?
| * @param {string} propertyName Property of the global data object. | ||
| * @param {string} [childPropertyName] Optional. Property of the object matched by `propertyName`. |
There was a problem hiding this comment.
Not to overcomplicate things, but why not use a path here like lodash.get accepts, in which case this could be much more flexible than parent [> child] only. We could also use get internally and return a clone of the value with some kind of not-found symbol perhaps as the default value?
There was a problem hiding this comment.
This occurred to me too, but I didn't want to stray too far from the IB. Using a Symbol for the not-found value is a good shout, I didn't think of that. I've applied your suggested change.
| * | ||
| * @param {string} propertyName Property of the global data object. | ||
| * @param {string} [childPropertyName] Optional. Property of the object matched by `propertyName`. | ||
| * @return {*} The global data object, or the child property value if provided. |
There was a problem hiding this comment.
In the past we've defined the last parameter as _global = global to allow for cleaner DI in testing. E.g. createTrackEvent
Any reason not to do the same here?
There was a problem hiding this comment.
I don't see any reason not to - I've added _global as the last parameter.
|
|
||
| it( 'should return the deep cloned global data object when no child property name is provided', () => { | ||
| const globalData = { foo: 'bar', baz: { qux: 'quux' } }; | ||
| global[ propertyName ] = globalData; |
There was a problem hiding this comment.
If we do end up using the real global here, we should ensure it's cleaned up afterEach to avoid leaking out of the test.
There was a problem hiding this comment.
Good shout, but as per your suggestion I've switched to a mock global object.
| email: 'admin@example.com', | ||
| name: 'admin', | ||
| picture: 'https://path/to/image', | ||
| full_name: 'Dr Funkenstein', |
There was a problem hiding this comment.
I'm glad we're keeping this 😄
|
Once sync'd with develop, E2E should run again as well. |
…me`. Also added the `_global` parameter to aid testing.
Thanks @aaemnnosttv, I've applied your suggestions. Re. Immer, it looks like it clones on write, but uses the original object until this point, so this change may still provide some value to Immer-enabled stores. |
aaemnnosttv
left a comment
aaemnnosttv
left a comment
There was a problem hiding this comment.
Thanks @techanvil – not sure what happened here but my comments didn't go through before 🙈
| * @param {string} propertyName Property of the global data object. | ||
| * @param {string} [path] Optional. Path to a value of the object matched by `propertyName`. |
There was a problem hiding this comment.
Thanks! I might not have been entirely clear in my previous suggestion, or you may be preferring to stick closer to the IB like you said, but I was actually suggesting to collapse both propertyName and the optional childPropertyName into a single path, so this is more of a light wrapper around _.get with error handling. I don't feel super strongly about it, but it seems unnecessary to allow for two path arguments when the one supports both.
| path = undefined, | ||
| _global = global | ||
| ) { | ||
| if ( ! _global[ propertyName ] ) { |
There was a problem hiding this comment.
This should probably be more specific to ! hasOwnProperty rather than a falsy value, no? I realize this is probably only going to be used with object values, but it's a bit inconsistent to throw an error only for the top-level path.
Following on my previous comment, WDYT about simplifying it to this?
function getGlobalData( path, _global ) {
const value = _.get( _global, path );
if ( value === undefined ) throw new Error();
return cloneDeep( value );
}_.get can't tell the difference between a set undefined value and missing value as the default value is returned for anything that resolves to undefined rather than something for sure missing/not-found, so my suggestion to add a symbol is probably over-complicating things 😅
2 participants
Summary
Addresses issue:
Relevant technical choices
I've only used the
getGlobalData()helper where the retrieved data is an object, not a primitive. Using in all cases adds a lot of extra error logging which complicates testing and doesn't add any real value, as the main point of this helper is to clone structured data.PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist