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

feat: experimental TypeSafeJSONModel #706

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

petermuessig
Copy link
Member

No description provided.

@Popescu-PfeifferMarc Popescu-PfeifferMarc self-assigned this May 20, 2023
@Popescu-PfeifferMarc
Copy link
Collaborator

@petermuessig could you please give me some feedback if you need anything else on this PR?

@petermuessig
Copy link
Member Author

Sorry, @Popescu-PfeifferMarc - I wanted to cross-check with @akudev - we will do so this week and keep you posted...

@petermuessig
Copy link
Member Author

Hi @Popescu-PfeifferMarc ,

we would suggest the following small adoptions:

Location: packages/ui5-types-typedjsonmodel
Package name: ui5-types-typedjsonmodel

TODO: check if inheriting from the JSONModel can simplify the usage of the types. Would be OK for @akudev and me.

One thing which may still be interesting is whether it should be rather consumed as .d.ts type definitions than as .ts typescript source code?

And I would also prefer a small example in the ui5-ts-simpleapp show-casing the usage of the typedjsonmodel and a description in the README.md of the ui5-types-typedjsonmodel package.

We may consider to move it into the official type definitions of UI5.

Would you still be interested in showing it in one of the next UI5ers live webinars?

@Popescu-PfeifferMarc
Copy link
Collaborator

@petermuessig Thanks for your feedback 👍 I will work on these improvements. Next 3 weeks for me are very full due to university exams so it might take some time. UI5ers live webinar would be cool 👍

@Popescu-PfeifferMarc
Copy link
Collaborator

Hi @petermuessig , I am now back from my university exams and quick vacation. I've added a Readme.md file with some explanations on how to use the Typed JSONModel and updated the example app to use the Typed JSONModel. I also renamed the package as you suggested.

I am not so sure about using the Typed JSONModel in the default UI5 type definitions, since it still has some caveats, such as not properly working with recursive types in all cases, not supporting deeply nested objects (default limit is 10 objects deep), as well as a couple other small things outlined in the Readme.md file.

Furthermore I believe that the pattern of defining an empty model and then adding properties to that model one by one is pretty common. This won't work out of the box with the Typed JSONModel since TypeScript can't infer the type of the model from the default values, since it is empty by default. Developers can fix this by manually specifying the type of the model in the generic parameter, but this requires manual effort.

Regarding the .d.ts file: As long as we aren't directly replacing the JSONModel type, I think .ts is the best option. This allows for incremental adoption across a codebase.

Could you please take a look at the showcase and the Readme.md file and give me some feedback?

Fixed commit message namings since commitizen didn't work for some reason
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

Successfully merging this pull request may close these issues.

2 participants