feat: add undefined type

[wusteven815]

• Closes Python - Differentiate null from undefined #549

[jnumainville]

Generally looks good. Just to verify, there is currently no spot where we want to allow None differentiated from UNDEFINED? I didn't see any and it's fine if we don't want to allow it anywhere but I want to make sure.

[mofojed]

I want us to be very careful when defining this type... @jnumainville perhaps you can provide some guidance as well.
Some test cases I would add specifically for testing the Undefined type:

1. Undefined == None should return True (similar to undefined == null in JS)
2. Undefined is None should return False (similar to undefined === null in JS)
3. String representation should print Undefined
4. Boolean representation should be False
5. Name of the object used should be Undefined (matching None)
6. The underlying class should be private (no need to expose Undefined and UNDEFINED, kind of confusing)

Right now with this solution, I think you just have 2 and 3 passing. A suggested solution:

class _Undefined: # Using `_` to mark as a private class, don't expose
    def __repr__(self):
        return None
    
    def __str__(self):
        return "Undefined"
    
    def __eq__(self, other):
        """
        Ensure if we use ==, we match objects of type None
        """
        return other is type(Undefined) or other == None


Undefined = _Undefined()

Testing that out:

print('Undefined == None: ', Undefined == None)
print('None == Undefined: ', None == Undefined)
print('Undefined is None: ', Undefined is None)
print('None is Undefined: ', None is Undefined)
print('str(Undefined): ', str(Undefined))
print('bool(Undefined): ', bool(Undefined))

That results with:

Undefined == None:  True
None == Undefined:  True
Undefined is None:  False
None is Undefined:  False
str(Undefined):  Undefined
bool(Undefined):  True

Write up unit tests to that effect as well.
@jnumainville or @mattrunyon , call out if I'm possibly missing anything here or any pitfalls you might see.

[mattrunyon]

We need to audit anywhere we're using is None probably because that is the preferred way in Python (since classes can implement an arbitrary equality check). So if we are changing defaults to Undefined (which I think makes sense so users can just use None in place of null) we need to make sure logic written around is None doesn't break and == None is what we want instead

[mattrunyon]

I also just checked that json.dumps({ "key": None }) results in { "key": null } in the JSON string

[mofojed]

Hmm I don't know if we could extend NodeEncoder to dump undefined when it encounters Undefined... maybe there's a way to skip the property at that point?

[mattrunyon]

We'd want to/have to skip it (or embed it specially if we really needed it). undefined isn't valid JSON

I don't think we should embed undefined. If we're already embedding/differentiating null, then we say no prop = undefined

We might need to modify our call to serialize elements so it drops undefined. If it were used in a nested config then it might not get dropped by default and cause a serialization error. If the encoder default doesn't let us drop keys (doesn't look like we can), then we would need to have some special string we know to replace on the client

[wusteven815]

For the __eq__ expression, other is type(Undefined) is always returning false for the examples. If we do Undefined == Undefined, the other == None part will recurse and call __eq__ for Undefined == None.

[mattrunyon]

I think you need isinstance(other, Undefined). type doesn't check anything from what I can tell and is requires pointing to the same object in memory. So a is type(a) will never be true because type(a) will create a new object every time it's called

[wusteven815]

I've added the following magic methods:

• __bool__ - so it's False
• __copy__ and __deepcopy__ - prevents a new instance from being created and breaking the is operator (e.g., used by dataclasses.asdict)

[mofojed]

I think with my other suggestion, this would be: start_name: str | Literal[Undefined] = Undefined.

[mattrunyon]

Looks like you can't get the type from a variable currently in Python. python/typing#769

[wusteven815]

IDEs/Pyright doesn't seem to like when Literal is used for values that are not primitive or enum. I've opted to type alias the class as UndefinedType (like NoneType, IDE will show the variable as a type instead of class). Unfortunately, UndefinedType() is still a valid way to create an instance. We could make it more difficult to create a new instance by doing:

_DISABLE_UNDEFINED_CONSTRUCTOR = False


class _Undefined:
    def __init__(self) -> None:
        if _DISABLE_UNDEFINED_CONSTRUCTOR:
            raise NotImplementedError()

    ...


Undefined = _Undefined()
UndefinedType: TypeAlias = _Undefined
_DISABLE_UNDEFINED_CONSTRUCTOR = True

[bmingles]

Suggested change

	The first non-None prop, or None if all props are None.
	The first non-nullish prop, or None if all props are None.

[bmingles]

Suggested change

	# no.w that the converter is set, we can convert simple props to strings
	# now that the converter is set, we can convert simple props to strings

[bmingles]

@wusteven815 Is there a basic example script to run to test this works?

[bmingles]

Left a few comments. Otherwise code changes look good to me.

[mattrunyon]

I'm wondering if this should be the default or if we should keep the defaults of None and only use undefined where it's actually needed. In the vast majority of components, treating None as undefined (i.e., dropping it from props) is perfectly fine. Distinguishing between the 2 seems to be the exception, not the rule.

Should we instead have a flag in the component decorator or constructor that says to differentiate null and undefined for rendering? I'm also thinking about future element components where a user implements their own components and must use undefined to prevent sending a bunch of nulls.

The other option would be to make the sentinel represent null instead. Although in the exception case, we want the user to just provide None for null probably, not the sentinel.

[mattrunyon]

Should add tests for comparing Undefined and None. All the cases Bender mentioned in an earlier comment would make good test cases

[mofojed]

@mattrunyon 's point is a good one - with these changes, we're having to make a lot of changes to accommodate Undefined, which is annoying when we add new features, and as a new dev using the library can be weird.
Thinking we should be able to specify in the component itself just which props we need to differentiate between null and undefined, and just have special cases for those.
Not 100% sure the best way to handle that. Most components just call component_element, but that already has *args and **kwargs filled in... maybe we have a special prop __nullable_props that we can pass in that gets read and removed when converting the props...?
E.g. Changing the signature of component_element to:

def component_element(name: str, /, *children: Any, _nullable_props: list[str], **props: Any) -> BaseElement:

Then a component can specify which props need to differentiate None vs. Undefined, e.g. for Calendar:

return component_element("Calendar", _nullable_props=['min_value'], **props)

[mofojed]

In theory after this change, this should be the only place that really needs to check for Undefined ... we should be removing it at this point so it shouldn't need to be known at the ElementMessageStream level or anything.

[wusteven815]

Continued by #1026, which simplifies the amount of code needed to be refactored and implemented review suggestions.