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

Use didOpen and didDestroy instead of deprecated onOpen and onDestroy #126

Merged
merged 5 commits into from
Oct 13, 2020

Conversation

limonte
Copy link
Member

@limonte limonte commented Sep 22, 2020

Closes #125

@limonte limonte requested a review from zenflow September 22, 2020 20:54
Copy link
Member

@zenflow zenflow left a comment

Choose a reason for hiding this comment

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

This will be totally broken with sweetalert2 version < 10.3.0

@limonte
Copy link
Member Author

limonte commented Sep 22, 2020

This will be totally broken with sweetalert2 version < 10.3.0

Right, should we release the new major with peerDependency "sweetalert2": "^10.3.0"? Or do you maybe know how it could be possible to use new hooks without breaking things for "sweetalert2": "< 10.3.0" users?

@zenflow
Copy link
Member

zenflow commented Sep 22, 2020

should we release the new major with peerDependency "sweetalert2": "^10.3.0"?

I don't like the idea of making a breaking change to deal with the hook name changes which supposed to be (and are themselves) backwards-compatible

Or do you maybe know how it could be possible to use new hooks without breaking things for "sweetalert2": "< 10.3.0" users?

Definitely possible.. I'm just not sure of the best way to go about it.

We could decide dynamically which hook names (original or updated) to use:

  1. If we receive options with updated hook names, use updated hook names (This takes 1st priority since "Old hooks are still called if defined, but only if the new name is not used simultaneously,")
  2. If we receive options with original hook names, use original hook names
  3. If we receive options with no hooks, use hook names according to Swal.version (use updated hook names where possible and otherwise use original hook names).

There are some tricky edge cases there though, like what if we get a mix? (e.g. onOpen and didDestroy)
That wouldn't happen often, but could, especially if user has separated concerns by writing (or just using) custom enhancers.
I guess we evaluate which hook name to use on an individual basis? (i.e. determine which hook name to use for opening and separately determine which hook name to use for destroying)

@limonte
Copy link
Member Author

limonte commented Oct 11, 2020

@zenflow should be good to go now, please review 🚀

@limonte
Copy link
Member Author

limonte commented Oct 11, 2020

I also added test-build-v9.html so we can easily and quickly test against the previous major.

@limonte
Copy link
Member Author

limonte commented Oct 13, 2020

@zenflow I'll go ahead and merge this PR. Please review when you have time, thank you!

@limonte limonte merged commit ce0f62d into master Oct 13, 2020
@limonte limonte deleted the didOpen-didDestroy branch October 13, 2020 12:41
limonte pushed a commit that referenced this pull request Oct 13, 2020
## [3.2.1](v3.2.0...v3.2.1) (2020-10-13)

### Bug Fixes

* use didOpen and didDestroy instead of deprecated onOpen and onDestroy ([#126](#126)) ([ce0f62d](ce0f62d))
@limonte
Copy link
Member Author

limonte commented Oct 13, 2020

🎉 This PR is included in version 3.2.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

limonte added a commit that referenced this pull request Nov 6, 2022
…stroy (#126)

* use didOpen and didDestroy instead of deprecated onOpen and onDestroy

* support legacy onOpen and odDestroy hooks as well

* chore: bump yarn.lock

* Add test-build-v9.html, add didOpen/didDestroy to test-build.html

* fix eslint issues
limonte pushed a commit that referenced this pull request Nov 6, 2022
## [3.2.1](v3.2.0...v3.2.1) (2020-10-13)

### Bug Fixes

* use didOpen and didDestroy instead of deprecated onOpen and onDestroy ([#126](#126)) ([547efd4](547efd4))
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.

Use didOpen and didDestroy instead of deprecated onOpen and onDestroy
2 participants