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

fix(types): improve type inference and correct some return types #1983

Merged
merged 3 commits into from
May 29, 2020
Merged

fix(types): improve type inference and correct some return types #1983

merged 3 commits into from
May 29, 2020

Conversation

rafaelss95
Copy link
Contributor

@rafaelss95 rafaelss95 commented May 28, 2020

  • With the current version ReadonlyMap isn't supported in inputOptions. This PR offers support for both versions: Map and ReadonlyMap;
  • DOM utilities could return null at some point and so it should be correctly represented with the type: HTMLElement | null;
  • SweetAlertOptions can accept two generic types now: one for preConfirm result and another for preConfirm callback value, e.g.:
// Note that both arguments are optional
const sweetAlertOptions: SweetAlertOptions<Promise<number>, string> = {
  // preConfirm: () => Promise.resolve(10),
  preConfirm: awesomeValue => Promise.resolve(10), // `awesomeValue` is inferred as `string`
  titleText: 'some fancy text',
};
// Previously we had to do some typecasts, now `sweetAlertResult.value` 
// is inferred automatically.
// In this case, it's inferred as `number`.
const { value } = await Swal.fire(sweetAlertOptions);
  • queue function accepts a generic type to get a typed response and also ReadonlyArray.
  • mixin's function parameter is required now;
  • update's function parameter now only accepts the UpdatableParameters described in
    export const updatableParams = [
  • All the keys in SweetAlertOptions are now readonly to better represent that even if you set something manually, it has no effect, like:
const sweetAlertOptions = { titleText: 'some fancy text' };
// With this PR, TS doesn't allow this: Cannot assign to 'titleText' 
// because it is a read-only property. You can achieve this effect using Swal.update(...)
sweetAlertOptions.titleText = 'no effect';


/**
* Normalizes the arguments you can give to Swal.fire() in an object of type SweetAlertOptions.
*
* Example:
* ```
* Swal.argsToParams(['title', 'text']); //=> { title: 'title', text: 'text' }
* Swal.argsToParams({ title: 'title', text: 'text' }); //=> { title: 'title', text: 'text' }
* Swal.argsToParams([{ title: 'title', text: 'text' }]); //=> { title: 'title', text: 'text' }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it was wrong. Can you confirm it, @limonte?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirming 👍

sweetalert2.d.ts Outdated
type SyncOrAsync<T> = T | Promise<T>;

type ValueOrThunk<T> = T | (() => T);

type UpdatableParameters =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to export this to perhaps use it in other packages like ngx-sweetalert2?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, please do export UpdatableParameters

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done :)

Copy link
Member

@limonte limonte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing contribution @rafaelss95!

Please export UpdatableParameters and I'll merge this PR.


/**
* Normalizes the arguments you can give to Swal.fire() in an object of type SweetAlertOptions.
*
* Example:
* ```
* Swal.argsToParams(['title', 'text']); //=> { title: 'title', text: 'text' }
* Swal.argsToParams({ title: 'title', text: 'text' }); //=> { title: 'title', text: 'text' }
* Swal.argsToParams([{ title: 'title', text: 'text' }]); //=> { title: 'title', text: 'text' }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirming 👍

sweetalert2.d.ts Outdated
type SyncOrAsync<T> = T | Promise<T>;

type ValueOrThunk<T> = T | (() => T);

type UpdatableParameters =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, please do export UpdatableParameters

@rafaelss95
Copy link
Contributor Author

In the last commit, I've updated some @default in docs.

Copy link
Member

@limonte limonte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great contribution, thank you so much @rafaelss95!

@limonte limonte merged commit d2e47cd into sweetalert2:master May 29, 2020
limonte pushed a commit that referenced this pull request May 29, 2020
## [9.13.3](v9.13.2...v9.13.3) (2020-05-29)

### Bug Fixes

* **types:** improve type inference and correct some return types ([#1983](#1983)) ([d2e47cd](d2e47cd))
@limonte
Copy link
Member

limonte commented May 29, 2020

🎉 This PR is included in version 9.13.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@rafaelss95
Copy link
Contributor Author

@limonte Pff, I just noticed that I forgot to add the SweetAlert prefix in UpdatableParameters 😞

@limonte
Copy link
Member

limonte commented May 29, 2020

@limonte Pff, I just noticed that I forgot to add the SweetAlert prefix in UpdatableParameters disappointed

No worries at all! just send another PR with the fix and I'll release the fix!

@limonte
Copy link
Member

limonte commented May 29, 2020

I believe 0 users started to use UpdatableParameters since I merged this PR :)

@rafaelss95 rafaelss95 deleted the fix/type-inference branch May 29, 2020 14:00
@rafaelss95
Copy link
Contributor Author

I believe 0 users started to use UpdatableParameters since I merged this PR :)

Haha, done. #1984.

@limonte
Copy link
Member

limonte commented Jun 7, 2020

Hey @rafaelss95

In our Angular integration this change seem to cause the issue: https://github.com/sweetalert2/ngx-sweetalert2/runs/747983015

src/lib/swal.component.ts:132:21 - error TS2540: Cannot assign to 'allowOutsideClick' because it is a read-only property.

132             options[prop] = this[prop as keyof this];

https://github.com/sweetalert2/ngx-sweetalert2/blob/bce7c7059c549aaab9586f3fa8270858f067b836/projects/ngx-sweetalert2/src/lib/swal.component.ts#L132

Should we revert Readonly or you can think of any better solution?

@rafaelss95
Copy link
Contributor Author

Hey @rafaelss95

In our Angular integration this change seem to cause the issue: https://github.com/sweetalert2/ngx-sweetalert2/runs/747983015

src/lib/swal.component.ts:132:21 - error TS2540: Cannot assign to 'allowOutsideClick' because it is a read-only property.

132             options[prop] = this[prop as keyof this];

https://github.com/sweetalert2/ngx-sweetalert2/blob/bce7c7059c549aaab9586f3fa8270858f067b836/projects/ngx-sweetalert2/src/lib/swal.component.ts#L132

Should we revert Readonly or you can think of any better solution?

Hello, I guess we can simply solve this without mutations, using reduce instead of forEach... something like this:

public get swalOptions(): SweetAlertOptions {
  return Array.from(this.touchedProps).reduce<SweetAlertOptions>((previousValue, currentValue) => ({
    ...previousValue,
    [currentValue]: this[currentValue as keyof this]
  }), {});
}

@toverux
Copy link
Member

toverux commented Jun 8, 2020

Hello,

I did not step in when you posted (cool PR btw!) but I think I should have ; my only concern was the addition of the readonly modifier. Technically, those properties aren't really readonly.
After all, readonly was added to signal frozen properties to the type system and we should be careful when using it strictly for documentation purposes.

For example, you may totally do :

const options: SweetAlertOptions = { title: '...' };

if (something) {
    options.onClose = () => { ... }
}

In fact, that's what I do in ngx-sweetalert2. It's just legit.

While I get that this provides some semantic information about how the options should be maipulated in general (and that's the good part and that's why I didn't intervene in the first place), it is also a constraint that's technically not really justified and now it caused problems.

I don't have a very strong opinion on this though.

What do you think ? :)

On another subject : some of the improvements on the types here were not made before because of backwards compatibility. For example, generic types with default values (in SweetAlertOptions) were not possible before. However, I think that now may be a good time, it has been implemented in TypeScript for a while now (along with Pick for update() I think). But I wanted to warn about this since it hadn't been mentioned yet.

You two have a great day!

@limonte
Copy link
Member

limonte commented Jun 8, 2020

I can see that the Readonly modifier created a bit of hassle in another integration: Basaingeal/Razor.SweetAlert2#724

If possible, I'd like to ask you @rafaelss95 to rollback the Readonly modifier as per @toverux's reasoning. Thank you in advance and apologies about not thinking about this carefully during the review!

@rafaelss95
Copy link
Contributor Author

Hello @toverux... I could argue that if the options are directly mutated it can hide bugs in first place, because if someone try to update a non updatable parameter, like below, it'll fail silently at static analysis. I know there's a warning in update's function, but in really we still lose that ability we gain from TS:

options.target = '#someId';

For example, you may totally do :

const options: SweetAlertOptions = { title: '...' };

if (something) {
    options.onClose = () => { ... }
}

In fact, that's what I do in ngx-sweetalert2. It's just legit.

Well, since there's the update method created for these situations, shouldn't it be better to use it instead?

if (something) {
  swal.update({
    onClose = () => { ... }
  })
}

Btw, what's the file/line that you do this there?


I can see that the Readonly modifier created a bit of hassle in another integration: Basaingeal/Razor.SweetAlert2#724

@limonte Hmm, I didn't go deep into this code, but shouldn't swal.update({ someOption: undefined }) be sufficient?

I mean... before it was: delete swalSettings[OPTION], after couldn't be: swal.update({ [OPTION]: undefined })?

If possible, I'd like to ask you @rafaelss95 to rollback the Readonly modifier as per @toverux's reasoning. Thank you in advance and apologies about not thinking about this carefully during the review!

No problem :). That all being said, I have no problem in create a PR rollbacking this. I'll do this ASAP. I just wanted to point that IMHO the readonly is a very powerful feature that we gain in TS side to prevent slightly bugs and, unless I'm missing something, all the possible mutations can be done via update method.

@toverux
Copy link
Member

toverux commented Jun 9, 2020

Well, since there's the update method created for these situations, shouldn't it be better to use it instead?

if (something) {
  swal.update({
    onClose = () => { ... }
  })
}

It's not the same use-case. You can legitimately mutate the options object before showing the swal. That's what I meant by saying "that's what I do in ngx-sweetalert2": I don't have this particular code, instead I was talking about my foreach function to mutate the object before showing the swal.

I know we can use reduce instead, and I prefer that too, but some may prefer the imperative style, or can't use reduce, like in my example. Instead, with readonly modifiers, they'd have do to something like this:

const options: SweetAlertOptions = {
    ...someCondition ? { onClose() { /* ... */ } } : {}
};

I am aware that these are rare use cases though.

@rafaelss95
Copy link
Contributor Author

rafaelss95 commented Jun 9, 2020

It's not the same use-case. You can legitimately mutate the options object before showing the swal. That's what I meant by saying "that's what I do in ngx-sweetalert2": I don't have this particular code, instead I was talking about my foreach function to mutate the object before showing the swal.

Hmm, when I read it yesterday I thought you're talking about a open swal. Sorry for the confusion.

matvejs16 pushed a commit to matvejs16/sweetalert2-fix that referenced this pull request Mar 29, 2023
…etalert2#1983)

* fix(types): improve type inference and correct some return types

* fixup! fix(types): improve type inference and correct some return types

* fixup! fix(types): improve type inference and correct some return types
matvejs16 pushed a commit to matvejs16/sweetalert2-fix that referenced this pull request Mar 29, 2023
## [9.13.3](sweetalert2/sweetalert2@v9.13.2...v9.13.3) (2020-05-29)

### Bug Fixes

* **types:** improve type inference and correct some return types ([sweetalert2#1983](sweetalert2#1983)) ([60f4bbf](sweetalert2@60f4bbf))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants