ngneat / dialog

👻 A simple to use, highly customizable, and powerful modal for Angular Applications
https://ngneat.github.io/dialog/
MIT License
394 stars 38 forks source link

fix: expose return type of dialog ref #87

Closed milo526 closed 1 year ago

milo526 commented 1 year ago

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[x] Other... Please describe: Fix for typing

What is the current behavior?

Issue Number: #86

What is the new behavior?

Dialogs that do have a result type defined will now expose said type in the afterClosed$ observable, instead of an any Observable.

Does this PR introduce a breaking change?

[x] Yes (but only in the typings, not on run-time)
[ ] No

Typing changes might be incompatible resulting in failed builds, this change should not introduce any runtime changes. Dialogs that do have a result type defined will now expose said type in the afterClosed$ observable, instead of an any Observable. Edit: This behavior has been updated in 6c0891d, components without a DialogRef property, or with a DialogRef property that does not have a Result defined, will now get a return type of any assigned. This is the same as the current behavior.

Other information

New tests for typing helpers have been introduced in this PR.

stackblitz[bot] commented 1 year ago

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

NetanelBasal commented 1 year ago

Did you test it locally and check that it works as expected in all cases?

milo526 commented 1 year ago

I don't know enough about this library to conclude what "all cases" is; the happy path works as expected; If you do provide a result type to the DialogRef, it is properly exposed in afterClosed$.

There is however the breaking change that was described, which makes the current tests fail.

Error: projects/ngneat/dialog/src/lib/specs/dialog.spec.ts:157:28 - error TS2322: Type 'unknown' is not assignable to type 'boolean'.

157         next: (result) => (value = result),
                               ~~~~~
Error: projects/ngneat/dialog/src/lib/specs/dialog.spec.ts:171:30 - error TS2322: Type 'unknown' is not assignable to type 'boolean'.

171           next: (result) => (value = result),

Since, in both cases, value is of type boolean while result is of type unknown.

It seems like the Result type should be any except for cases where it is well defined as another type. However, I have no clue on how to achieve this.

This MR fixes the happy path but not the edge cases.

To clarify, modals without a ref property (where ExtractRefProp<Modal> yields a never type), will after this MR have a result type of unknown whereas they used to have an any result type.

jemand771 commented 1 year ago

I'm using this and it works fine for me. The only issue is that the result can be undefined when closing via the default close button or backdrop, which doesn't get caught by typescript

edit: not a problem introduced by this PR, but one that only becomes visible because of it. somewhere in that chain, the result should be optional or | undefined. Alternatively, maybe there's a way to specify a default result that gets triggered from pressing esc / clicking the close button / clicking the backdrop ?

milo526 commented 1 year ago

This push was a simple rebase to get our downstream fixed for another branch, please disregard these changes.
I still want to take some time to fix this MR but that will be at a later time, thank you for you patience.

milo526 commented 1 year ago

The last changes fix the previous issues with the test and remove the "breaking change".

This PR does not introduce any run-time changes.

I added some regression tests for the ExtractData and ExtractResult test helpers.

NetanelBasal commented 1 year ago

@milo526 the breaking change should be included in the commit

milo526 commented 1 year ago

Breaking change footer is added to the commit.