Don't crash when setting JS theme value to null#16210
Merged
philipp-spiess merged 4 commits intomainfrom Feb 4, 2025
Merged
Conversation
0aeec86 to
7b82024
Compare
null'null
RobinMalfait
approved these changes
Feb 4, 2025
Member
RobinMalfait
left a comment
There was a problem hiding this comment.
Small suggested change, otherwise looks good!
Co-authored-by: Robin Malfait <malfait.robin@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #16035
In v3 it was possible to unset a specific color namespace by setting doing something like this:
This pattern would crash in v4 right now due to the theme access function not being able to work on the red property being a
null. This PR fixes this crash.However it leaves the behavior as-is for now so that the red namespace defined via CSS will still be accessible. This is technically different from v3 but fixing this would be more work as we only allow unsetting top-level namespaces in the interop layer (via the non-
extend-theme-object). I would recommend migrating to the v4 API for doing these partial namespace resets if you want to get rid of the defaults in v4:Test plan
The crash was mainly captured via the test in
compat/config.test.tsbut I've added two more tests across the different levels of abstractions so that it's clear whatnullshould be doing.