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

[0.15-?] Edge Promise.finally not defined #3427

Closed
erakis opened this issue Mar 1, 2019 · 9 comments
Closed

[0.15-?] Edge Promise.finally not defined #3427

erakis opened this issue Mar 1, 2019 · 9 comments

Comments

@erakis
Copy link
Contributor

erakis commented Mar 1, 2019

Software version

Quasar: 0.15.5
@quasar/app (v1+ only):
OS: Windows 10
Node: v8.9.3
NPM: 5.5.1
Browsers: Microsoftt Edge 17
iOS: N/A
Android: N/A
Any other software related to your bug:

JsFiddle

https://jsfiddle.net/1oqygeh9/2/

What did you get as the error?

object doesn't support property or method finally

image

What were you expecting?

I was expecting to see the message Finally fired like
image

The polyfill do not seems to work properly.

Could it be because the polyfill only fill when Promise is not supported at al instead of also test for only missing finally method ? If yes, this could be the source of the problem, because Edge < 18 support Promise but not the method finally.

What steps did you take, to get the error?

I load the jsfiddle page with Microsoft Edge 17 and I simply get the error.

I have two workmates with Windows 10 who their Windows 10 is not in developer mode and they seem to be stuck at Edge version 17, Windows update says it is up to date!

For Edge version 18, this is working properly but as I know the Promise.finally is now natively supported. Ref ChakraCore #3520

image
Ref: https://en.wikipedia.org/wiki/Microsoft_Edge


I'm still using an old version of quasar, but for now we are stuck and can't update to new version. Maybe in a couple of week we will.

Also, I already set the IE support flag in quasar.conf.js.

module.exports = ctx => ({
  supportIE: true,
});
@pdanpdan
Copy link
Collaborator

pdanpdan commented Mar 1, 2019

supportIE only polyfills what quasar uses. Just replace .finally(something), which is not well supported, with .then(something, something) or with .catch(() => true).then(something)

@erakis
Copy link
Contributor Author

erakis commented Mar 1, 2019

supportIE only polyfills what quasar uses. --@pdanpdan

This is weird as the IE polyfill contains the prototype for adding the finally method but quasar does not use them at all. I search in all the code of the quasar repo and no trace of finally usage... Why put that in ?

.catch(() = {}).then(something) --@pdanpdan

From what I've read and test, this version will not work for a special case that I have. I need to throw an error in the catch, so the next then will not be fired. BUT, when using a finally it get fired properly.

An ex :

Promise.resolve()
   .then( () => ) {
       // Show a loading message...
       throw new Error('foo');
    })
   .catch((ex) => {
       // Log the error...
       
       // Rethrow it
       throw ex;
   })
   .finally(() => {
       // Hide the loading message...
    })

Do you know another way to get it working as a finally ?

@pdanpdan
Copy link
Collaborator

pdanpdan commented Mar 1, 2019

In your case just hide loading message in catch before throw.

@erakis
Copy link
Contributor Author

erakis commented Mar 1, 2019

Yes, your're right @pdanpdan, this is a solution I had considered, but I would have liked to avoid duplicate code. Now I've no choice.

Is there a chance the polyfill ie-compat be fixed to test for the support of Promise.finally instead of only testing for the support of Promise ?

I would be more than please to make the PR but I'm not sure to really understand how this polyfill work and how it detect the missing support.

@panstromek
Copy link
Contributor

Usual workaround for .finally is to put .then(onFinally, onFinally) (from mdn)
after the last catch clause.

I would suggest you to not bet on polyfills when the workaround is that simple. Polyfilling and transpilation is very complex. Adding such a simple thing is not really worth it considering the risk of possible breakage.

Another thing is that 0.15 is really old version and full focus is on 1.0 now. We can't spend time patching legacy version.

@erakis
Copy link
Contributor Author

erakis commented Mar 4, 2019

Usual workaround for .finally is to put .then(onFinally, onFinally) (from mdn)
after the last catch clause. – @panstromek

Thank you, but did you read this reply ? I' can't use .then(onFinally, onFinally), this will clearly not work for my needs. But thank you for your help.

Another thing is that 0.15 is really old version and full focus is on 1.0 now. We can't spend time patching legacy version. – @panstromek

As for the version 1.0 of quasar, I already explained that I can't update for now.

@erakis erakis closed this as completed Mar 4, 2019
@panstromek
Copy link
Contributor

panstromek commented Mar 4, 2019

@purell I've read your reply, but I think I mean something different.

This behaves exactly the same as your finally version 😉

Promise.resolve()
   .then( () => ) {
       // Show a loading message...
       throw new Error('foo');
    })
   .catch((ex) => {
       // Log the error...
       
       // Rethrow it
       throw ex;
   })
   .then(() => {
       // Hide the loading message...
    }, () => {
       // Hide the loading message...
    })

(better to extract the function of course)

The only difference for finally are params and return values - to make this behave exactly the same as finally, you would need to re-throw the error in last callback and return resolved value in previous one. This is because finally passes values through, but then callbacks passes what you return from them. But from my understanding you don't need this last bit.

@erakis
Copy link
Contributor Author

erakis commented Mar 4, 2019

So to be clear, both versions are working but both duplicate the code anyway :( I would have liked to see finally worked.

  1. You version
Promise.resolve()
   .then(() => {
       // Show a loading message...
       throw new Error('foo');
    })
   .catch((ex) => {
       // Log the error...
       
       // Rethrow it
       throw ex;
   })
   .then(() => {
       // Hide the loading message...
       console.log('Hide message 1');
    }, () => {
       // Hide the loading message...
       console.log('Hide message 2');
    });
  1. Other version with duplicate
Promise.resolve()
   .then(() => {
       // Show a loading message...
       throw new Error('foo');
    })
   .catch((ex) => {
       // Log the error...

       // Hide the loading message...
       console.log('Hide message');
       
       // Rethrow it
       throw ex;
   })
   .then(() => {
       // Hide the loading message...
       console.log('Hide message');
    });

@nothingismagick
Copy link
Contributor

@purell - if you can you confirm that this is still happening in quasar 0.17, then I imagine we would dig into it after 1.0 launches. We have no plans to continue to support 0.15

On a side note, .finally is called REGARDLESS of a catch, that's why its called finally.

The finally() method can be useful if you want to do some processing or cleanup once the promise is settled, regardless of its outcome. 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants