Breaking change: close() as instance method #1379

gverni · 2019-01-11T18:50:43Z

Closes #919

I noticed the TODO comment on:

sweetalert2/src/instanceMethods/_main.js

Line 46 in f76808e

 constructor.closePopup(innerParams.onClose, innerParams.onAfterClose) // TODO: make closePopup an *instance* method 

and I took the opportunity of the fix for issue #919 to refactor close() as instance method.

I also removed the aliases closeModal() and closeToast() that were not used / documented anywhere.

@limonte @zenflow let me know if I went too far and i'm happy to implement to implement the fix for close() as static method.

coveralls · 2019-01-11T18:59:40Z

Pull Request Test Coverage Report for Build 4516

0 of 0 changed or added relevant lines in 0 files are covered.
22 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.04%) to 90.499%

Files with Coverage Reduction	New Missed Lines	%
dist/sweetalert2.js	22	90.5%

Totals
Change from base Build 4511:	0.04%
Covered Lines:	1120
Relevant Lines:	1189

💛 - Coveralls

zenflow · 2019-01-11T19:02:12Z

@gverni Yup, I think we want this change for the upcoming major release. Note that it is a breaking change to the API and thus not technically just a refactor.

We need to do this for all the static methods that make more sense to be instance methods.. see #1373

limonte

Looks very good!

closeModal and closeToast were not deprecated and removing then would be an unexpected breaking change. Even though they were not documented, some people might use them. Let's not break anything without strong reasons for that.
Please add the test proving that the desired functionality from Calling swal.close() does not call the onClose callback #919 works as expected

resolve the promise with {} (no value or dismiss)

limonte · 2019-01-12T19:40:55Z

Congrats, everyone! 💪

gverni · 2019-01-12T19:47:11Z

:-) Congratulations!!!

…

On Sat, 12 Jan 2019, 19:40 Limon Monte, ***@***.***> wrote: [image: image] <https://user-images.githubusercontent.com/6059356/51077696-afbe8800-16b2-11e9-97ea-6b798e45eff1.png> Congrats, everyone! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1379 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQTDdLHkI0tKEi1DUwHlPGvvz7qBWpCFks5vCjpKgaJpZM4Z7_nT> .

gverni · 2019-01-13T18:53:10Z

Hi @limonte sorry it took me so long, but I was trying to find the most correct way to achieve:

resolve the promise with {} (no value or dismiss)

What I did, is to add resolve and reject into a new innerProps.innerFunctions object. To be honest, I'd rather create a new innerMethods object and add those to that, if you like this approach.

Alternatively, I tried also to save references to reject / resolve in one of the properties of Swal, but this will expose these methods, and I personally did not like it (though it obviously works).

To be honest, I'm not really happy with that, but I couldn't think of any better way of achieving that goal. Feel free to let me know if there is one.

gverni · 2019-01-13T19:00:30Z

The failure on CI is obviously due to the fact that i'm always resolving the promise at the end of close(). Before fixing that, I just want to be sure i'm on the right path with this approach.

gverni · 2019-01-14T13:57:04Z

I really didn't like to use privateProps to hold the promise's resolve / reject. So I added a privateMethods object to hold these (and future) private methods.

I also moved the logic for handling resolve / reject directly within close() method

gverni · 2019-01-14T16:37:05Z

Given #1383 I can simplify even more the prototype of the close() and keep only the resolveValue

limonte · 2019-01-14T17:03:00Z

Given #1383 I can simplify even more the prototype of the close() and keep only the resolveValue

That's a possible change, yeah. But let's agree on it before doing any changes on this PR.

Created a new object to hold privateMethods (similar to existing privateProps) and added to this object the methods for swalPromise

* Added two optional arguments to close() to handle resolve value and reject value * Added privateMethods object to call resolve / reject for swal promise

gverni · 2019-01-17T19:41:50Z

@limonte @zenflow I think i'm done with this PR. Feel free to review it.

limonte

Good work, @gverni!

Do I understand correctly that this PR doesn't contain any breaking changes?

If so, please remove "Breaking change" form the PR title.

test/qunit/methods/close.js

gverni · 2019-01-18T10:09:26Z

thanks @limonte!

Do I understand correctly that this PR doesn't contain any breaking changes?

The breaking change is in the fact that now if you call Swal.close() it does resolve the promise (with {}), whereas before it didn't. Do you agree?

BREAKING CHANGE: close() as instance method (#1379)

# [8.0.0](v7.33.1...v8.0.0) (2019-01-19) * BREAKING CHANGE: remove getButtonsWrapper() ([c93b5e3](c93b5e3)) * BREAKING CHANGE: close() as instance method (#1379) ([2519c17](2519c17)), closes [#1379](#1379) * BREAKING CHANGE: replace deprecated `jsnext:main` with `module` in package.json (#1378) ([1785905](1785905)), closes [#1378](#1378) * BREAKING CHANGE: drop Bower support (#1377) ([cb4ef28](cb4ef28)), closes [#1377](#1377) * BREAKING CHANGE: remove withNoNewKeyword enhancer (#1372) ([f581352](f581352)), closes [#1372](#1372) * BREAKING CHANGE: remove swal.noop() ([40d6fbb](40d6fbb)) * BREAKING CHANGE: rename $swal2-validationerror -> $swal2-validation-message (#1370) ([9d1b13b](9d1b13b)), closes [#1370](#1370) * BREAKING CHANGE: inputValidator and preConfirm should always resolve (#1383) ([fc70cf9](fc70cf9)), closes [#1383](#1383) * BREAKING CHANGE: remove setDefault and resetDefaults (#1365) ([97c1d7c](97c1d7c)), closes [#1365](#1365) * BREAKING CHANGE: remove extraParams (#1363) ([5125491](5125491)), closes [#1363](#1363) * BREAKING CHANGE: remove showValidationError and resetValidationError (#1367) ([50a1eff](50a1eff)), closes [#1367](#1367) * BREAKING CHANGE: remove useRejections and expectRejections (#1362) ([f050caf](f050caf)), closes [#1362](#1362) * BREAKING CHANGE: dismissReason: overlay -> backdrop (#1360) ([d05bf33](d05bf33)), closes [#1360](#1360) * BREAKING CHANGE: drop Android 4.4 support (#1359) ([c0eddf3](c0eddf3)), closes [#1359](#1359) ### Features * **api:** add update() method ([#1186](#1186)) ([348e8b7](348e8b7)) ### BREAKING CHANGES * close() as instance method (#1379) * drop Android 4.4 support (#1359) * replace deprecated `jsnext:main` with `module` in package.json (#1378) * drop Bower support (#1377) * remove withNoNewKeyword enhancer (#1372) * remove swal.noop() * rename $swal2-validationerror -> $swal2-validation-message (#1370) * inputValidator and preConfirm should always resolve (#1383) * remove setDefault and resetDefaults (#1365) * remove extraParams (#1363) * remove showValidationError and resetValidationError (#1367) * remove useRejections and expectRejections (#1362) * dismissReason: overlay -> backdrop (#1360) * remove getButtonsWrapper()

limonte · 2019-01-19T10:17:14Z

🎉 This has been shipped in version 8.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

gverni requested review from limonte and zenflow January 11, 2019 18:51

limonte requested changes Jan 12, 2019

View reviewed changes

gverni changed the title ~~Refactor close() as instance method~~ [WIP] Refactor close() as instance method Jan 13, 2019

gverni changed the title ~~[WIP] Refactor close() as instance method~~ Refactor close() as instance method Jan 14, 2019

gverni changed the title ~~Refactor close() as instance method~~ Breaking change: close() as instance method Jan 14, 2019

limonte mentioned this pull request Jan 17, 2019

BREAKING CHANGE: inputValidator and preConfirm should always resolve #1383

Merged

gverni added 14 commits January 17, 2019 17:07

Implemented close() as instance method

44ecf8d

Added close to instance methods

5c7556e

Removed close from static methods

7ba72ff

Use closePopup as istance method

f7c0e11

Removed closeModal and closeToast (not used)

29d8740

Removed comment

9638e41

Removed comment

18895bb

Restored closeModal and CloseToast

e69a5d5

Added test cases for close() method

f361934

Added innerFunctions to privateProps

ba382a5

Added promise resolve/reject to innerFunctions

7cff8db

Resolve the promise at the end of close()

7606b5a

Moved innerFunction to newly created privateMethods

d0225a5

Created a new object to hold privateMethods (similar to existing privateProps) and added to this object the methods for swalPromise

Handle resolve / reject in close() method

4ba0e21

* Added two optional arguments to close() to handle resolve value and reject value * Added privateMethods object to call resolve / reject for swal promise

gverni added 4 commits January 17, 2019 17:14

Added promise's resolve / reject to privateMethods

9b6ae80

Added test for close() resolve value

544ec3f

Removed reject for swal promise private method

c903a0c

Remove rejection handling from close() method

bad3ab5

gverni force-pushed the fix/swal-close branch from 6a668c5 to bad3ab5 Compare January 17, 2019 18:07

limonte requested changes Jan 18, 2019

View reviewed changes

test/qunit/methods/close.js Outdated Show resolved Hide resolved

Removed wrong comments

cea3b81

limonte approved these changes Jan 18, 2019

View reviewed changes

limonte merged commit ca9d197 into master Jan 18, 2019

limonte deleted the fix/swal-close branch January 18, 2019 11:43

limonte mentioned this pull request Jan 18, 2019

Calling swal.close() does not call the onClose callback #919

Closed

limonte pushed a commit that referenced this pull request Jan 18, 2019

BREAKING CHANGE: close() as instance method (#1379)

b180fba

BREAKING CHANGE: close() as instance method (#1379)

limonte pushed a commit that referenced this pull request Jan 18, 2019

BREAKING CHANGE: close() as instance method (#1379)

2519c17

BREAKING CHANGE: close() as instance method (#1379)

Disane87 mentioned this pull request Jan 29, 2019

Closing dialogs with dynamic components sweetalert2/ngx-sweetalert2#100

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Breaking change: close() as instance method #1379

Breaking change: close() as instance method #1379

gverni commented Jan 11, 2019 •

edited by limonte

coveralls commented Jan 11, 2019 •

edited

zenflow commented Jan 11, 2019

limonte left a comment •

edited

limonte commented Jan 12, 2019 •

edited

gverni commented Jan 12, 2019 via email

gverni commented Jan 13, 2019 •

edited

gverni commented Jan 13, 2019

gverni commented Jan 14, 2019

gverni commented Jan 14, 2019

limonte commented Jan 14, 2019

gverni commented Jan 17, 2019

limonte left a comment •

edited

gverni commented Jan 18, 2019 •

edited

limonte commented Jan 19, 2019

Breaking change: close() as instance method #1379

Breaking change: close() as instance method #1379

Conversation

gverni commented Jan 11, 2019 • edited by limonte

coveralls commented Jan 11, 2019 • edited

Pull Request Test Coverage Report for Build 4516

💛 - Coveralls

zenflow commented Jan 11, 2019

limonte left a comment • edited

Choose a reason for hiding this comment

limonte commented Jan 12, 2019 • edited

gverni commented Jan 12, 2019 via email

gverni commented Jan 13, 2019 • edited

gverni commented Jan 13, 2019

gverni commented Jan 14, 2019

gverni commented Jan 14, 2019

limonte commented Jan 14, 2019

gverni commented Jan 17, 2019

limonte left a comment • edited

Choose a reason for hiding this comment

gverni commented Jan 18, 2019 • edited

limonte commented Jan 19, 2019

gverni commented Jan 11, 2019 •

edited by limonte

coveralls commented Jan 11, 2019 •

edited

limonte left a comment •

edited

limonte commented Jan 12, 2019 •

edited

gverni commented Jan 13, 2019 •

edited

limonte left a comment •

edited

gverni commented Jan 18, 2019 •

edited