Skip to content
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

sap.ui.model.Model.prototype.createBindingContext return type are wrong #379

Open
nlunets opened this issue Sep 26, 2022 · 6 comments
Open

Comments

@nlunets
Copy link
Member

nlunets commented Sep 26, 2022

sap.ui.model.Model.prototype.createBindingContext says it returns a Context | undefined.

however the ClientModel already returns Context | null it seems :/

That's pretty annoying if you want to have proper null checks :)

@akudev
Copy link
Contributor

akudev commented Sep 27, 2022

Hm. The situation is like this:

  • v2.ODataModel code returns undefined or null (or a context, of course; and claims it returns context or undefined)
  • v4.ODataModel returns always a context, apparently (and claims so)
  • the original ODataModel returns undefined or null (and claims @see Model)
  • ClientModel returns null (and claims @see Model)
  • model.MetaModel returns null (and claims @see Model)
  • Model is abstract and claims it returns undefined

So what exactly is your request? that the base class "Model" should document the return type as "null" instead of "undefined"? (the one in error case of course) That would seem to make sense.

Or that in addition the the original ODataModel admits it might also return "undefined" and the v2.ODataModel admits it might also return "null"??

@codeworrior
Copy link
Member

@uhlmannm and @pksinsik can you please take a look at this? V4 never returning null or undefined is okay for TypeScript, but the others should align on a signature that they inherit from the base class.

I would assume that for the widely used models, compatibility "rulz". Extending the base signature to both, undefined and null might be the only way to fix this. But maybe you're more optimistic reg. a leaner solution (e.g. as v2 ODataModel does not document the null, can it replace it with undefined?).

@nlunets
Copy link
Member Author

nlunets commented Sep 28, 2022

So what exactly is your request? that the base class "Model" should document the return type as "null" instead of "undefined"? (the one in error case of course) That would seem to make sense.

I wish for every model to confirm what they return properly at least :) so yes returning null instead of undefined feels better.

Regarding v4, it actually throws error when something is not ok which might be contrary to the philosophy of the models in general.

@pksinsik
Copy link

@codeworrior We should not change the cases where v2.ODataModel#createBindingContext returns null to return undefined; same is true for ClientModel. Reason is - compatibility...
If you say TypeScript really requires it we may enhance the return value documented for Model#createBindingContext.

@akudev
Copy link
Contributor

akudev commented Sep 28, 2022

If you say TypeScript really requires it we may enhance the return value documented for Model#createBindingContex

Isn't correctness also a good reason to change documentation?

@pksinsik
Copy link

we will work on the topic "fix jsdoc for null value for a context resp. optional oContext parameters across APIs for base model, client models, v2.ODataModel classes" via the existing incident 2270138264 already opened by @nlunets

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants