-
Notifications
You must be signed in to change notification settings - Fork 30
feat(dts-generator): add TypedJSONModel #516
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
base: main
Are you sure you want to change the base?
Conversation
test-packages/openui5-snapshot-test/output-dts/sap.ui.core.d.ts
Outdated
Show resolved
Hide resolved
rebase now against the primary error in node 18 and update the expected test result in openui5-snapshot-test with "npm run re-generate" |
Ah, that was just putting your code to the right places - thanks a lot for the implementation, it's great to get this in! In addition to the comments above, as I told you, please document what to do when a fix is needed, esp. how to update the code in the dts-generator. My prior mail had the description how I split type definitions and implementation. For a small fix or extension you'd probably just apply the changes to what we already have in the dts-generator and in the OpenUI5 repo, not do the full splitting exercise again. Also, we need some documentation for users of the TJM. We only have the JSDoc in the types themselves (good but hard to discover when you don't know the feature) and the sample app deep inside this repo. If you write up some markdown, we can find a good place. E.g. in the gh-pages branch of this repo, where we can get it in immediately (and where you can develop the docu as PR). That place would also be a good one to mention the limitations. Including the relatively esoteric technical ones within the model you already described, but more importantly the real-life ones outside the TJM itself which are more likely to be seen by users, like that in XMLView bindings there is of course no type safety provided by using a typed model (so you can still bind to non-existing properties) etc. |
83a40ad
to
30e8bcc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, this looks good to me now. Only the version dependency of adding the TJM types to the dts file is missing.
@codeworrior , @petermuessig if you want to have a look as well...
AFAICS and besides the already raised comments (Andreas, Frank) it looks good. Can't wait to see it live... |
Add the type definitions for the
TypedJSONModel
and adapt the generator to insert them to the core d.ts file.Related to #509 – thanks for your submission, @akudev !
I added tests for
TypedJSONModel
, which can be executed separately vianpm run test:typed-json-model
, so they can be added to CI when appropriate.