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

Remove unhandled promise rejection code. #72

Closed
MicahZoltu opened this issue May 26, 2015 · 19 comments
Closed

Remove unhandled promise rejection code. #72

MicahZoltu opened this issue May 26, 2015 · 19 comments
Labels

Comments

@MicahZoltu
Copy link

https://github.com/zloirock/core-js/blob/master/modules/es6.promise.js#L100-L106

The Promise spec doesn't have this for a good reason, please remove it or at least make it easy to globally disable. There is discussion about what to do about this situation https://github.com/promises-aplus/unhandled-rejections-spec/issues but it isn't getting much traction because without a finalizer you can't be sure when someone is done with a promise.

Most of the proposed solutions assume that you are using promises in a very particular way and while it is not terribly uncommon, the promise spec was intentionally written to make promises more generalized than that.

The use cases where this "feature" falls apart is when you have fulfilled promises that are saved and used later. A simple example is this:

let foo = Promise.reject(new Error("failure"));
setTimeout(() => foo.then(_ => console.log("success"), _ => console.log("failure")), 1);

In this example, foo will be seen as having failed without any rejection handler and an error will be logged. However, you can see that the promise was checked for success/failure at a later time (possibly much later). If you use promises in this way right now you are hammered with failure messages even though your code is working perfectly fine.

The unhandled promise rejection logic, as currently implemented, discourages people from writing code that treats promises as a first-class citizens and instead delegates them to callback aggregators/transformers. A promise is an object that may have a useful lifetime that far exceeds the the executor. They are intended to store the result of the executor (success or failure) until they are garbage collected.

A more concrete example would be pre-fetching some web resources that you expect you'll need later. A rejection is a reasonable (and intended) state for a promise to be in and you may not attach a then to the promise until much later in your application when you need the data. At that point it would be appropriate to check for an error, you just happened to have prefetched the data to improve the responsiveness of your application.

@zloirock
Copy link
Owner

Native Chromium and Io.js Promise has support the same unhandled rejection tracking, if I'll remove it you will have this behavior in them. Your example shouldn't cause unhandledRejection event / message in core-js.

\cc @benjamingr

@MicahZoltu
Copy link
Author

In my real world case the rejection handler is attached much later (not on next loop). I am prefetching some data and saving the promise. Much later, when the user interacts with something, I use the promise. At this point I will tell the user about the error (if there is one) or leverage the result (if there is one).

io.js promise and native chromium aren't behaving to spec then I don't believe. I don't, personally, believe that this library should be following them.

If everyone truly believes this should be part of the spec then I encourage them to put together a proposal or discuss one of the existing proposals at the link I provided. Going out and doing something different isn't the right way to get things into the language IMO. The proposals are argued against for very good reasons, not just because people want to be curmudgeons.

@sebmck
Copy link

sebmck commented May 26, 2015

io.js promise and native chromium aren't behaving to spec then I don't believe.

This is incorrect. Following the spec does not mean you cannot add additional behaviour and semantics.

@MicahZoltu
Copy link
Author

I am of the opinion that to meet a spec you can't have visible side effects beyond what is specified in the specification. As an extreme/silly example, would you consider it meeting a spec if my promise implementation erased your hard drive, but it still met all of the requirements of the spec?

I believe a spec to be not only the positive space, but the negative space as well. A spec defines what something should do as well as what it should not do by the lack of mention of everything else.

@sebmck
Copy link

sebmck commented May 26, 2015

@Zoltu If you want to be pedantic about it, yes.

Is the current behaviour somehow a blocker for you (if it's destructive and prevents you from doing something then it should be a bug that's reported)? What's the disadvantage besides seeing some errors in your console?

@MicahZoltu
Copy link
Author

The problem is that my logs are flooded by these errors because I treat rejected promises as first class citizens. It is effectively making my log useless because I am writing promise driven code that is heavily dependent on promises, some portion of which start in a rejected state with no handler.

@zloirock
Copy link
Owner

I don't, personally, believe that this library should be following them.

core-js follows not only ES6 spec.

I do not think you have chosen the right place for this issue. As I wrote above, native Chromium and Io.js Promise have the same behavior and even if I did remove it not saves you of your problem. Rather, it is your architectural problem. If you are not satisfied with this behavior - you can wrote issues here: WhatWG browser proposal, NodeJS proposal, io.js discussion, chromium bugtracker.

@MicahZoltu
Copy link
Author

I have posted on a couple of those. I'll post on the others as well.

@vkurchatkin
Copy link

@Zoltu also read this thread: nodejs/node#256. I've also expressed some concerns about this issue at some point, but the solution is quite simple.

@benjamingr
Copy link
Contributor

i'm not sure I understand your use case but the prefetching thing has been discussed several times to death.

I saw @vkurchatkin helped you suppress it for a single promise in another thread. I encourage you read the proposal for the unhandled rejection tracking hooks. It's also not the correct place to discuss it here as core-js is merely behaving like all other implementations.

I'm still 100% in favor of opt-out instead of opt in because automatically suppressed errors are a pain to debug. Generally we've received very positive feedback about these hooks.

@screendriver
Copy link

Hi there,

I have the same issue. I am using Oboe.js to parse a really really large JSON file

const promises = [];
oboe('http://domain/my-file.js')
  .node('items.*', item => {
    // parseItem() returns a rejected Promise because of invalid JSON items
    promises.push(parseItem(item));
  })
  .done(() => {
    Promise.all(promises).then(() => {
      doSomething();
    });
  })

my Browser console gets flooded with Unhandled promise rejection.

What's really strange: if I don't use this polyfill and use the native promise implementation builtin in modern browsers it behave differently. In Firefox Developer Edition everything works without the error messages and in Chrome I get flooded with Uncaught (in promise).

So what's the solution for this? Ignoring the message? I mean if this behavior is really in the official promise spec then promises in asynchronous code does not make really sense for me 😕

@zloirock
Copy link
Owner

@screendriver unhandled rejection tracking in near time will be added to ECMAScript spec. core-js and Chrome supports it, FF - not.

You can ignore this message or, better, simple change your code to something like

const items = [];
oboe('http://domain/my-file.js')
  .node('items.*', item => {
    items.push(item); // store items, not promises
  })
  .done(() => {
    Promise.all(items.map(i => parseItem(i))).then(() => { // parse items here
      doSomething();
    }).catch(e => console.error(e)); // handle your error
  })

@screendriver
Copy link

@zloirock thank you for your response. So the behavior of core-js is the final one that will also be implemented natively in all browser?

Unfortunately your solution does not work in my case. parseItem() is a function that makes an asynchronous operation as well (callback based) and therefore needs to be wrapped in a promise. Because of memory issues, especially on mobile devices, I can't save all items in an in-memory array. That's the reason why I use Oboe.js. Do you have any idea to solve this?

const parseItem = item => {
  return new Promise((resolve, reject) => {
    const onSuccess = () => {
      // ...
      resolve();
    };
    const onFailure = error => {
      // ...
      reject(error);
    };
    insertIntoStorage(onSuccess, onFailure);
  });
};

const promises = [];
oboe('http://domain/my-file.js')
  .node('items.*', item => {
    promises.push(parseItem(item));
  })
  .done(() => {
    Promise.all(promises).then(() => {
      doSomething();
    });
  })

@zloirock
Copy link
Owner

So the behavior of core-js is the final one that will also be implemented natively in all browser?

Something like.

Do you have any idea to solve this?

You can just add .catch to your parseItem call

    promises.push(parseItem(item).catch(e => /* handle parsing error */ e));

BTW you can change default unhandled rejection handler - for example, just remove logging.

@screendriver
Copy link

Hmm but if I make a promises.push(parseItem(item).catch(e => /* handle parsing error */ e)); then my Promise.all().catch() don't get called 😕

It seems for me that the whole thing is not well thought out or I am missing something.

@Zoltu what is your solution regarding this?

@benjamingr
Copy link
Contributor

I'd answer it if you posted a question in Stack Overflow. I don't like the way people use GH issues as a support channel though. If I get @zloirock's permission I'll answer it here :)

@zloirock
Copy link
Owner

@benjamingr welcome, it's not critical for me. If someone will found an answer here, he will not add one more question about it :)

For questions about core-js possible use gitter.

@screendriver
Copy link

StackOverflow? Here you go 😉

Thank you for your help guys!

@benjamingr
Copy link
Contributor

Added an answer. Let me know what you think.

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

No branches or pull requests

6 participants