promises-aplus / promises-spec

An open standard for sound, interoperable JavaScript promises—by implementers, for implementers.
https://promisesaplus.com/
Creative Commons Zero v1.0 Universal
1.84k stars 164 forks source link

nbd.js as a conforming implementation #116

Closed Aintaer closed 11 years ago

Aintaer commented 11 years ago

nbd.js implements Promise/A+ as an inheritable trait that can be used with its Class.mixin() to make every instance of that class .then()able.

In addition, it also provides a .promise() that allows it to work with jQuery's $.when().

briancavalier commented 11 years ago

Hey @Aintaer, can you let us know how to run the Promises/A+ test suite against your impl? I cloned, npm installed (which did install the test suite), and then ran npm test which seemed to run your unit tests, but not the Promises/A+ test suite.

Let us know and we'll get this merged once we can see the tests pass. Thanks!

Aintaer commented 11 years ago

Hi @briancavalier, I couldn't figure out how to integrate the tests directly into our existing Karma setup, so to run the Promise/A+ tests I had to do it manually with:

node_modules/promises-aplus-tests/lib/cli.js test/lib/promise-adapter.js 

If you have any suggestions on integrating with Karma, please let me know!

domenic commented 11 years ago

@Aintaer if you modify your package.json npm test to be

whatever-it-is-currenty && promises-aplus-tests test/lib/promise-adapter.js

that'll work nicely.

Note that inside npm scripts you don't need the leading ./node_modules/karma/bin/, ./node_modules/requirejs/bin/, or ./node_modules/uglify-js/bin/ :)

Aintaer commented 11 years ago

@domenic oh that's a nice tip, thanks!

Although I could add the secondary call to npm test, ideally I'd want to kick off the suite from Karma itself, so as to gain its benefits.

I'm going to look into it a bit more and add the promise test call if I can't integrate with karma directly.

briancavalier commented 11 years ago

Thanks @Aintaer. I just tried it, and I'm having trouble running the tests on a fresh clone. Here's what I'm doing:

  1. git clone https://github.com/behance/nbd.js.git
  2. cd nbd.js
  3. ./node_modules/promises-aplus-tests/lib/cli.js test/lib/promise-adapter.js

And the resulting error:

/private/tmp/nbd.js/node_modules/promises-aplus-tests/lib/cli.js:46
        throw new Error("Error `require`ing adapter file " + filePath);
              ^
Error: Error `require`ing adapter file /private/tmp/nbd.js/test/lib/promise-adapter.js
    at adapterObjectFromFilePath (/private/tmp/nbd.js/node_modules/promises-aplus-tests/lib/cli.js:46:15)
    at Object.<anonymous> (/private/tmp/nbd.js/node_modules/promises-aplus-tests/lib/cli.js:8:15)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Function.Module.runMain (module.js:497:10)
    at startup (node.js:119:16)
    at node.js:901:3

Could you try a fresh clone and see if you are able to run them using the above?

I did a little looking around and I noticed that nbd's own modules use fully qualified module ids when referencing each other. For example, nbd/trait/promise wants to load nbd/util/async, but references it by require()ing 'nbd/util/async', rather than '../util/async'. I understand you may have reasons for doing it that way, but it may be causing the issue here. I've not seen a node package reference itself that way, and I know that for AMD, fully qualifying internal module references makes it harder for the package consumer to move the module around.

Either way, let us know if you are able to run the tests on a fresh clone using the 3 steps above. If I need to do something differently, or another step, just let me know.

Thanks!

Aintaer commented 11 years ago

@briancavalier Yeah I run into the same issue on a fresh clone. The way it was working on my development repo was because I had npm link to establish nbd as the fully qualified root.

The reason why we originally used fully qualified module names was so that we could redirect the nbd path to the test specs directory where the dependencies specified in the actual modules would pull in their tests to ensure everything along the dependency chain was tested.

Since I think we've outgrown that original setup, as of behance/nbd.js@9fc9bf20443a6b2d5ac73dd19c25e51bb8b5fd38, the library only uses relative paths and should be testable with a npm test, which will run the Promise/A+ tests after nbd's own tests.

briancavalier commented 11 years ago

@Aintaer, thanks for the fast turnaround! I just fetched the latest and ran npm test and everything looks good. Promises/A+ test suite passes. Well done.

@domenic, feel free to merge if you don't have any additional concerns.