zloirock / core-js

Standard Library
MIT License
24.55k stars 1.65k forks source link

Remove unhandled promise rejection code. #72

Closed MicahZoltu closed 9 years ago

MicahZoltu commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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

vkurchatkin commented 9 years ago

@Zoltu also read this thread: https://github.com/nodejs/io.js/issues/256. I've also expressed some concerns about this issue at some point, but the solution is quite simple.

benjamingr commented 9 years ago

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 commented 9 years ago

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 :confused:

zloirock commented 9 years ago

@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 commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

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 :confused:

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 commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

StackOverflow? Here you go :wink:

Thank you for your help guys!

benjamingr commented 9 years ago

Added an answer. Let me know what you think.