Rework Dex placeholder objects

[scheibo]

Context: #5475 (comment)

[scheibo]

This should probably throw instead of creating a placeholder.

Originally posted by @Zarel in #5475

The simple way to check this is to change this to throw and run a bunch of smoke tests to see what happens.

However, I don't know that we can make getEffectByID throw if not found - its called by getEffect, which seems like its supposed to be able to handle arbitrary input? At least, thats the reason you gave on #5194 for not allowing me to skip the hasOwnProperty call.

[Zarel]

I thought getEffect would manually create an Effect object if not found, rather than delegating to getEffectByID.

[scheibo]

I thought getEffect would manually create an Effect object if not found, rather than delegating to getEffectByID.

For that to happen, we'd want there to be a third, private method which is called internally by getEffect and getEffectByID and returns null/undefined/sentinel such that getEffect can then construct a placeholder and getEffectByID can throw, but this seems unnecessary (adding a catch to getEffect to avoid the third method is a nonstarter).

The biggest reason we don't just return undefined is because https://github.com/tc39/proposal-optional-chaining is still Stage 1 after approximately eight years of literally everyone asking for it.

Originally posted by @Zarel in #5475

If we want optionals, I'd prefer an Optional wrapper class with Optional.of(effect) or Optional.empty() because then you only new up a small object instead of the whole shebang, but I guess that defeats your purpose of the placeholder object making it so you dont need to care the object doesnt exist. With Typescript as a null or undefined checker though, you basically get all the same benefits of @NonNull/@Nullable checking, which makes returning undefined safe AFAICT (just less convenient, because without chaining the compiler will force you to handle missing values differently).

Though can we just return nullObject instead of a named {exists: false} placeholder? You still get a well behaving Effect and don't waste cycles constructing a garbage object. The only downside is that obviously the name wont match - how much does that matter here? Really, my main concern with this 'placeholder pattern' is that it is substantially less efficient than the traditional null object pattern you were going for given how relatively expensive it is to make the placeholder.

[Zarel]

I'm fine with a single cached placeholder as a stopgap. But we might want to actually fix it.

throwing might be appropriate for all getEffect calls? The only one is when it's used by stuff like /data, which we might want to manually wrap in a try-catch?