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

Addition of 'then' function in 0.26.0 makes Ramda an incomplete thenable #2751

Open
tmcw opened this Issue Dec 21, 2018 · 8 comments

Comments

Projects
None yet
6 participants
@tmcw
Copy link

tmcw commented Dec 21, 2018

Code to replicate:

const R = require("ramda");
(async function() {
  return await R;
})();
// (node:46376) UnhandledPromiseRejectionWarning:
// TypeError: `then` expected a Promise, received function () { [native code] }

We’re running into this issue with Observable, in which we await library loads from dynamic import() calls, but it’ll also happen for anyone who’s using the esm module to do dynamic import() in Node.js now, and for anyone who uses Node.js dynamic import whenever they ship it. Here’s the dynamic import failure case:

(async function() {
  const R = await import("ramda/es/index.js");
})().catch(e => console.error(e));
// TypeError: `then` expected a Promise, received function () { [native code] }
// at _assertPromise (/Users/tmcw/tmp/a/node_modules/ramda/es/internal/_assertPromise.js:6:11)

This code works properly in 0.25.0 and breaks in 0.26.0 and beyond, because await works like:

  1. Await the imported code
  2. Await the result of that import

Before, because Ramda was an object, the second await didn't do anything. Now, because it's a 'thenable object' - an object with a then method - await tries to await its eventual value and triggers the error.


I'm not a maintainer or contributor of this project, so my 2c on how to resolve this is likely to miss something - my only real guess for a solution would be to rename then to something else.

@CrossEye

This comment has been minimized.

Copy link
Member

CrossEye commented Dec 21, 2018

Uggh!!!

The whole point of adding then was to get us away from having to do anything important with Promises.

@Bradcomp, @buzzdecafe, anyone else in @ramda/core, any suggestions?

I really, really, really hate Promises!

@Bradcomp

This comment has been minimized.

Copy link
Member

Bradcomp commented Dec 21, 2018

Heh, that's pretty funny. Yet another example on how toxic Promises are to the ecosystem.

I don't know much about dynamic imports, but it seems like it would be unnecessary for Ramda, given that all the exports are static. Can you provide a use case for awaiting Ramda? I'm not suggesting this is a non-issue, I would just like some additional context.

Even though I was the one who created R.then, I think the best solution is just to remove the two Promise related functions and be done with it. It's clear that going down that path leads to heartache and pain, and it's plenty simple to re-create those functions in user space.

Maybe we should just add a section to the readme with reference implementations along with an explanation on why we don't recommend them. Or modify map and chain to dispatch to then.

I'm not a maintainer or contributor of this project, so my 2c on how to resolve this is likely to miss something - my only real guess for a solution would be to rename then to something else.

You aren't really missing anything other than that it's really tough to come up with good names for things, especially when they are related to other things that already have names. We couldn't use catch for the failure case because it's a reserved keyword and had a big discussion on the name to choose instead.

@tmcw

This comment has been minimized.

Copy link

tmcw commented Dec 21, 2018

I don't know much about dynamic imports, but it seems like it would be unnecessary for Ramda, given that all the exports are static.

Sure thing: if an application lazy-loads ramda. What people call code splitting. So if you have a page deep in your application that needs to use Ramda, but you don't want to load Ramda as soon as someone loads the website. It's a common and very effective optimization.

@Bradcomp

This comment has been minimized.

Copy link
Member

Bradcomp commented Dec 21, 2018

That makes sense. I knew about lazy loading but just didn't fully work out the implications for Promises. This is a bummer. 😢

@paldepind

This comment has been minimized.

Copy link
Member

paldepind commented Dec 30, 2018

I think the best solution is just to remove the two Promise related functions and be done with it.

I agree 👍

Due to the design of the promise spec it seems troublesome to add a then property to anything that's not a promise.

@evan-king

This comment has been minimized.

Copy link

evan-king commented Jan 3, 2019

Another options that I believe will read well (enough) in context is renaming then to afterward.

@foxbunny

This comment has been minimized.

Copy link
Contributor

foxbunny commented Jan 3, 2019

Another options that I believe will read well (enough) in context is renaming then to afterward.

Or maybe call it awkward itstead, because that's what it is. :)

@CrossEye

This comment has been minimized.

Copy link
Member

CrossEye commented Jan 3, 2019

Another options that I believe will read well (enough) in context is renaming then to afterward.

Or even andThen 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment