ibm-js / delite

HTML Custom Element / Widget infrastructure
http://ibm-js.github.io/delite/
Other
68 stars 28 forks source link

Use ES6 Promise API rather than dojo/Deferred #350

Closed wkeese closed 9 years ago

wkeese commented 9 years ago

This PR converts the delite code to use the ES6 promise API, as shimmed by https://github.com/calvinmetcalf/lie. Note that shim could be replaced by any equivalent shim, or by a plugin that only loads the shim if necessary, which would fully resolve ibm-js/sdk#5.

This includes one API change: events now have a setLoadDeferred() method rather than a loadDeferred property. Probably that function should have a better name, like setChildInfo(). Anyway, setLoadDeferred()'s parameter is either a plain object, or a thenable (aka Promise). The change is because ES6 Promises don't have resolve() and reject() methods, so it makes sense for the event handler to specify the whole Promise.

require(["delite/register", "delite/DisplayContainer", "lie/dist/lie", "dojo/request"/*, ...*/], 
  function (register, DisplayContainer, Promise, request/*, ...*/) {
  document.addEventListener("delite-display-load", function(event) {
     event.setLoadDeferred(new Promise(function (resolve, reject) {
      // build a file name from the destination
      request(event.dest+".html", function (data) {
         // build a child from the data in the file
         var child = a_parse_function(data);
         // resolve with the child 
         resolve({ child: child });
      }));
  });
});

Internally, the changes to match ES6 Promise spec are:

Other notes:

cjolif commented 9 years ago

Has it been verified if UMD/browserify is not breaking grunt-amd-build build?

wkeese commented 9 years ago

I haven't yet. Actually I can't get the normal build (without my change) to work, because it gets stuck looking for sizzle.js. Maybe @clmath can try it.

tkrugg commented 9 years ago

I think I faced the same issue before and this solved it: https://github.com/ibm-js/angular-delite-example/commit/73d69644b506167dc594ffe5568e9670f6ad5f65

wkeese commented 9 years ago

OK, I finally got a build working, and it all seems fine. The lie code is embedded into the layer and the ExampleWidget.html file is working.

wkeese commented 9 years ago

Christophe said he was OK with this and I haven't seen anyone else's comments. So I pushed it as 68f42090956febe60943096f3273de360834ddeb, after addressing the comments above and doing some more cleanup.

edchat commented 9 years ago

I am testing this with dapp now, sorry I was not able to test it sooner due to vacation. And it seems like the code in DisplayContainer.show() after the call to this.emit("delite-display-load", event); is getting into the first Promise.resolve(child).then() before dapp calls setChild(child), so the child is null. I will try to setup a test outside of dapp to show the problem, but I am not sure how this code is supposed to be working, and it is not clear in the debugger, I am seeing "Mutation (async)" in the call stack when it gets in there, but I am not sure what that means. Is there some timeout on this Promise or something?

edchat commented 9 years ago

It looks like you can see the problem if you put a setTimeout around the call to event.setChild() in the DisplayContainer event unit test.

wkeese commented 9 years ago

@edchat, with the new API you need to call setChild() synchronously, but you can pass in a Promise that resolves asynchronously.

edchat commented 9 years ago

Thanks wkeese, I will give it a try, by the way, it would be good to have a test doing that, I will plan to add one.

wkeese commented 9 years ago

Oh actually, I'll just modify one of the two existing calls to setChild() (in the DisplayContainer test) to pass in a promise.