Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor: Add data component for editor initialization to replace __unstableInitialize action. (fixes #15403) #15444
Refactor: Add data component for editor initialization to replace __unstableInitialize action. (fixes #15403) #15444
Changes from all commits
17b9283
4d7a9c2
ee2f8aa
8f81af4
03041b9
03acbf3
84ab1f4
973f7f8
4a602b8
301e675
4e11e87
508e222
d5e9efd
68f680f
09db379
fd56104
521664c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might think
null
serves better as representing an unset value (this is also what was used previously). Unless for some reasonnull
can't be used as the argument ofuseRef
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used empty string because of a habit I have of using the same type for empty values (
previousOpenedSidebar.current
would have a string value). I think it's a good signal of what type to expect?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, to me, we're representing an explicitly empty value, which is the purpose that a null type serves (reference), and isn't really a semantic guarantee of an empty string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other option we may have here is to not set any value (just initialize with
useRef()
)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although that may mean
previousOpenedSidebar.current
will be undefined (I'd have to check).Edit: just confirmed
useRef()
results in the expected{ current: undefined }
value.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'd still think
null
would be preferred overundefined
, as the distinction between the two is in intentionality / explicitness of emptiness (vs. merely "unset").There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, in your reference is stated:
And the accompanying example illustrates where null is used to reference an empty value for something that would return an object otherwise. That's typically how I use
null
as the initial value for a variable, when the type returned would be an object as opposed to a primitive. Of course this isn't a hard and fast "rule" because context is important (i.e. it may not always be good to initialize value that represents a number primitive with0
) but in general I find value when reading code with initialized values that communicate expected type.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow. You're suggesting to distinguish it based on whether it's
typeof 'object'
vs. some other primitive type?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. In this case
previousOpenedSidebar.current
will always return a string. So it's empty representation would be an empty string (keeping type equality). IfpreviousOpenedSidebar.current
is expected to return an object, then it's empty representation would benull
(which seems to be the use-case for how MDN describes usingnull
).On line 75 I also followed the same line of thought by resetting
previousOpenedSidebar.current
to an empty string:gutenberg/packages/edit-post/src/components/editor-initialization/listener-hooks.js
Line 75 in 521664c
I think
null
would be also be an appropriate initial value to use in the case where a value may represent any number of data types.