Should JSMol::get_mol return null in case it fails to generate a molecule?

[ptosco]

Intended audience for this poll

JavaScript developers actively using RDKit MinimalLib (rdkit-js).
I am tagging a few that I know of here, but there are certainly more:
@MichelML @adam-of-barot @StLeonidas @MariaDolotova @greglandrum

The problem

Currently, JSMol::get_mol() always returns a JSMol object to JavaScript, even when it actually failed generating a molecule from input (i.e, when MinimalLib::mol_from_input() returned nullptr):
https://github.com/rdkit/rdkit/blob/f1d8b06fa10180bc0de88b46089b15c093bcee46/Code/MinimalLib/minilib.cpp#L729

In such cases, the JSMol object will simply wrap a null pointer.
Therefore, currently JavaScript developers need to check that the JSMol object is valid by calling its is_valid() method before attempting to do anything with it.
Even if the JSMol object is invalid, its delete() method must still be called from JavaScript to avoid leaking memory.

The proposal

To me, the above seems unnecessary overhead.
My proposal is to have JSMol::get_mol() return null to JavaScript upon failure, and deprecate the is_valid() method.
Thus, JavaScript developers will only need to check for the get_mol() return value to not be null before using the molecule (which most likely already do anyway), and they would not need to call its is_valid() method anymore.
Additionally, they would not need to bother deleting invalid JSMol objects as there would be no more invalid JSMol objects, since get_mol() returns null when it fails to generate a molecule.
Initially the is_valid() method would be deprecated and would always return true.
In a future release the is_valid() method would be removed.

Please note that the above is a breaking change

If the above proposal is accepted, existing code that currently calls is_valid() without previously checking that the returned molecule is not null will fail with

Uncaught TypeError: Cannot read properties of null (reading 'is_valid')

[adam-of-barot]

I think this would be a favorable change, at least from a consistency standpoint.
This way the JS behaviour would be consistent with that of C++ and Python.

[ptosco]

@adam-of-barot Thank you for your feedback, Ádám.

[MichelML]

@ptosco I share the same opinion as @adam-of-barot , it's a good idea (although it will be a painful change hehe)

[ptosco]

@MichelML Thanks for spreading the word! Hopefully not too painful, as I guess most developers are already checking for the returned object to be non-null before calling is_valid() on it ;-)

[MichelML]

@atfera @RamziWeslati please vote on this I know you've been working hard on https://github.com/iktos/molecule-representation

[MichelML]

@Phil-DS any thoughts on this?

[MichelML]

@Oloren-AI , thoughts ?

[Phil-DS]

I do agree with the change to get_mol, returning null on a bad generation. Cleaner code for bad generations, and is a bit more logical to follow.

On a separate point from an Angular point of view, being able to compare to null vs calling the is_valid function in a template is more performant than without, due to how Angular checks for changes.

[ptosco]

@Phil-DS Thank you for your feedback!