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

🐦 👊 promise #65

Merged
merged 1 commit into from
Aug 26, 2015
Merged

🐦 👊 promise #65

merged 1 commit into from
Aug 26, 2015

Conversation

iamdustan
Copy link
Contributor

Handles my current environment that resulted in #64. There is probably a better test for this than what I have done though...

@ericclemmons ericclemmons changed the title 🐦 👊 promise [WIP] 🐦 👊 promise Aug 10, 2015
@@ -159,7 +159,8 @@ export default class Resolver extends React.Component {
const factory = resolve[name];
const value = factory(props);

if (value instanceof Promise) {
// duckpunched promise
if (value instanceof Promise || typeof value.then === 'function') {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you switch to double-quotes to match to match the rest of the coding style? (I guess I forgot to .eslintrc this)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eslint was eating it for me. ran eslint src (instead of npm run lint) to get the error. All updated now.

@ericclemmons
Copy link
Owner

@iamdustan What library are you using for fetching? I want to test out a few to ensure this works with Promise.all on the server.

@ericclemmons ericclemmons added this to the v2 milestone Aug 10, 2015
@ericclemmons
Copy link
Owner

Related to #64.

@iamdustan
Copy link
Contributor Author

I am using https://github.com/matthew-andrews/isomorphic-fetch which requires an external Promise implementation. I likely am experiencing some issues between require('es6-promise').polyfill(), babel’s core-js rewriting, and multiple node modules which are also compiled with babel.

@ericclemmons
Copy link
Owner

I was talking to people about this today & they recommended wrapping pretty much everything in Promise.resolve, which is supposed to solve the duck-punch problem for us?

Have you heard about this? As in, Promise has to exist for this project to run.

@ericclemmons
Copy link
Owner

Hah! I got bit by this bug that's actually an issue with the compiled code, I'd wager!
If I @resolve("test", () => "Howdy"), it's fine.

If I @resolve("test", () => Promise.resolve("Howdy")), I get:

{ 'Symbol(record)_1.ael8nu52kh9kvs4i': 
   { p: [Circular],
     c: [],
     a: undefined,
     s: 1,
     d: true,
     v: 'Howdy',
     h: false,
     n: true } }

Plus, test instanceof Promise === true, but React Resolver isn't resolving it.

@ericclemmons
Copy link
Owner

This needed a bit more finagling, but this works:

          if (value && typeof value.then === "function") {
            nextState.pending[name] = value;
          } else {

I'd like to combine the two together for clarity. I should have this resolved tomorrow.

@ericclemmons ericclemmons merged commit 788e182 into ericclemmons:master Aug 26, 2015
@ericclemmons
Copy link
Owner

@iamdustan Thanks for this. This was huge, especially considering there are major issues with instanceof Promise between Babel-ified projects.

@ericclemmons ericclemmons changed the title [WIP] 🐦 👊 promise 🐦 👊 promise Aug 26, 2015
@iamdustan
Copy link
Contributor Author

🙌 woohoo!

@iamdustan iamdustan deleted the duckpunch branch August 27, 2015 02:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants