Async to promises. writeGltf, writeBinaryGltf, writeFile -- callbacks to Promises #135

JoshuaStorm · 2016-07-13T13:34:15Z

~~Not~~ ready for review.

I am currently switching some files to be Promises instead of using callbacks, but I've hit a few snags which have resulted in me doing some anti-patterns that I wanted to ask a few questions about to resolve.

I'll be adding inline comments so it is easier to see what I am talking about.

JoshuaStorm · 2016-07-13T14:12:16Z

specs/lib/writeBuffersSpec.js

+                expect(gltf.buffers.CesiumTexturedBoxTest.uri).toEqual('CesiumTexturedBoxTest.bin');
+                readFile(outputBufferPath)
+                    .then(function(outputData) {
+                        expect(bufferEqual(outputData, bufferData)).toBe(true);


I am fairly certain I shouldn't have to be nesting .then like this, but I am unsure how I could write this in order to have outputData accessible by the second .then function

I have attempted the code below, but that results in outputData being undefined.

writeGltf(gltf, options) .then(function() { expect(gltf.buffers.CesiumTexturedBoxTest.extras).not.toBeDefined(); expect(gltf.buffers.CesiumTexturedBoxTest.uri).toEqual('CesiumTexturedBoxTest.bin'); }) .then(function() { readFile(outputBufferPath); }) .then(function(outputData) { expect(bufferEqual(outputData, bufferData)).toBe(true); done(); });

@JoshuaStorm
You need to return a promise to do chaining like that.

writeGltf(gltf, options) .then(function() { expect(gltf.buffers.CesiumTexturedBoxTest.extras).not.toBeDefined(); expect(gltf.buffers.CesiumTexturedBoxTest.uri).toEqual('CesiumTexturedBoxTest.bin'); return readFile(outputBufferPath); }) .then(function(outputData) { expect(bufferEqual(outputData, bufferData)).toBe(true); done(); });

@lasalvavida
I appreciate the quick responses to these questions. That makes a lot of sense but I seemed to have just been gleaning over this fact while reading the documentation.

JoshuaStorm · 2016-07-14T16:23:31Z

specs/lib/gltfPipelineSpec.js

+            })
+            .catch(function(err) {
+                done();
+                throw err;


The three above tests are failing

The spy is never being triggered and I am not exactly sure why. I do a similar spy setup for writeGltfSpec and writeBinaryGltfSpec that work fine.

@lasalvavida or @lilleyse, if you have a minute could you briefly look over these tests and see if there is anything I'm obviously missing

I think it's because you remove the args to the fake callback, so args[0] is undefined.

I am fairly certain spy.calls.first() accesses the args passed to the function spied on.

See writeGltfSpec for example

Doing a few console.log traces, it seems that for some reason the the processFileToDisk and processJSONToDisk are resolving before the outputJson in writeGltf

I'm guessing there is something wrong with my promises

I'm guessing there is something wrong with my promises

That is true. Hang tight for a little bit; I'm almost done fixing this

…ne into async-to-promises

lasalvavida · 2016-07-14T17:18:23Z

@JoshuaStorm This branch should be clean again

lasalvavida · 2016-07-14T17:53:01Z

I'm seeing errors in Travis, but this is clean locally for me

JoshuaStorm · 2016-07-14T18:35:38Z

Ignore the most recent commit, it was off-base

I'll look more into this

JoshuaStorm · 2016-07-14T19:37:55Z

I have resolved the Travis failure that occurred due to attempting to write to a directory without the directory first being made.

However, this solution is a bit hacky:

I want to use both synchronous and asynchronous fs-extra functions. As such, I cannot just promisifyAll of fs-extra since that results in an error being thrown when a synchronous function is called.

A simple solution would be just to require('fs-extra) and promisify the individual functions I want to use as promises, but this causes an issue where I cannot spy on the individual function (there may be a simple resolution to this that I am unaware of)

For example, if I do

var fsExtra = require('fs-extra');
var outputJson = Promise.promisify(fsExtra.outputJson);

and attempt to do spyOn(fsExtra, 'outputJson'), the spy does not seem to be triggered by calling the promisified outputJson

My current solution is a bit clunky, and inefficient: I use both var fsExtra = require('fs-extra') and var fsPromise = Promise.promisifyAll(require('fs-extra')) so I can use the synchronous and asynchronous functions, while also being able to spyOn(fsPromise, 'outputJson')

lasalvavida · 2016-07-14T19:48:27Z

@JoshuaStorm do the following:

fsExtra.outputJsonAsync = Promise.promisify(fsExtra.outputJson);
...
spyOn(fsExtra, 'outputJsonAsync')

JoshuaStorm · 2016-07-14T19:49:25Z

I knew I had to be forgetting something straight forward like that.
Thanks!

JoshuaStorm · 2016-07-15T13:54:19Z

This should be good for review now!

lilleyse · 2016-07-15T16:21:50Z

lib/writeGltf.js

    // Create the output directory if specified
    if (createDirectory) {
        outputPath = path.join(path.dirname(outputPath), 'output', path.basename(outputPath));
+        fsExtra.ensureDirSync(path.dirname(outputPath));


You don't need this because outputJson will create the dir if it doesn't exist.

I had to do this due to writeSource using fs.writeFile (not fsExtras), but a better solution is probably just to use fsExtras in writeSource, so I'll go ahead and do that.

Ah ok, that solution seems good.

lilleyse · 2016-07-15T16:21:59Z

specs/lib/getBinaryGltfSpec.js

+var vertexShaderPath = './specs/data/boxTexturedUnoptimized/CesiumTexturedBoxTest0VS_Binary.glsl';
+
+describe('getBinaryGltf', function() {
+


Remove line.

lasalvavida · 2016-07-15T18:21:34Z

Async tests should use

expect(promise, done).toResolve();

https://github.com/AnalyticalGraphicsInc/agi-cesium-people/tree/node-coding-guide/NodeCodingGuide#testing-differences-from-cesium

This is more robust and will keep failing tests from falling through promises.

JoshuaStorm · 2016-07-15T18:26:22Z

@lasalvavida Good catch, I forgot about that.

Just to make sure I do these properly:

This

        writeGltf(gltf, options)
            .then(function() {
                expect(gltf.shaders.CesiumTexturedBoxTest0FS.extras).not.toBeDefined();
                expect(gltf.shaders.CesiumTexturedBoxTest0FS.uri).toEqual('CesiumTexturedBoxTest0FS.glsl');
                return fsReadFile(outputFragmentShaderPath);
            })
            .then(function(outputData) {
                expect(bufferEqual(outputData, fragmentShaderData)).toBe(true);
                done();
            });

should become

        var promise = writeGltf(gltf, options)
            .then(function() {
                expect(gltf.shaders.CesiumTexturedBoxTest0FS.extras).not.toBeDefined();
                expect(gltf.shaders.CesiumTexturedBoxTest0FS.uri).toEqual('CesiumTexturedBoxTest0FS.glsl');
                return fsReadFile(outputFragmentShaderPath);
            })
            .then(function(outputData) {
                expect(bufferEqual(outputData, fragmentShaderData)).toBe(true);
                done();
            });
        expect(promise, done).toResolve();

Right?

Thanks!

lasalvavida · 2016-07-15T18:29:16Z

Make sure you don't call done like in your example.

I would also write it a little more compactly, like this:

expect(writeGltf(gltf, options)
    .then(function() {
        expect(gltf.shaders.CesiumTexturedBoxTest0FS.extras).not.toBeDefined();
        expect(gltf.shaders.CesiumTexturedBoxTest0FS.uri).toEqual('CesiumTexturedBoxTest0FS.glsl');
        return fsReadFile(outputFragmentShaderPath);
    })
    .then(function(outputData) {
        expect(bufferEqual(outputData, fragmentShaderData)).toBe(true);
    }), done).toResolve();

lasalvavida · 2016-07-15T20:21:05Z

lib/compressTextureCoordinates.js

 var AttributeCompression = Cesium.AttributeCompression;
 var Cartesian2 = Cesium.Cartesian2;
+var defined = Cesium.defined;


defined should be at the bottom of this section.

lasalvavida · 2016-07-15T20:23:09Z

I think bin/gltf-pipeline.js may need updating since processFileToDisk is a promise now.

lasalvavida · 2016-07-15T20:25:30Z

If node already waits for all promises to terminate, it may be as simple as putting a comment that says that so there's no confusion.

lasalvavida · 2016-07-15T20:26:51Z

lib/readGltf.js

 var fs = require('fs');
+var path = require('path');
+var Promise = require('bluebird');
+var readFile = Promise.promisify(fs.readFile);


Make this fsReadFile for consistency with the node guide.

lasalvavida · 2016-07-15T20:34:14Z

Those are my last few comments; this looks awesome @JoshuaStorm!

lasalvavida · 2016-07-18T14:23:38Z

@lilleyse This does change the public API; do we want to merge this now or wait until after the first alpha release?

lilleyse · 2016-07-18T14:31:01Z

@lilleyse This does change the public API; do we want to merge this now or wait until after the first alpha release?

I was thinking about this too. It's probably best to wait.

I have another question though. Should our public API return promises at all, or should those functions take callbacks? Internally the callback would just wrap around the promise. It seems like callbacks are still the standard for most other node libraries. @mramato do you have thoughts on this?

mramato · 2016-07-18T14:50:34Z

Our public API should absolutely return promises. There are two ways to go here:

Return promises and not provide callbacks at all. (The ideal solution)
Support both, which is what I've seen other libraries do. Basically, any public function that normally returns a promise would take an optional callback function as it's final argument. Passing in the callback will cause it to act like a traditional callback function and not return a promise. We could easily write a single promiseToCallback utility function that handles this for us and we would have to call it wherever we return a promise from the public API.

While 2 wouldn't be a ton of code, it does add some additional overhead and clutter the API a bit. We would also have to add an extra test for every public API function to cover it. My gut says to go with 1 until enough people ask for 2. It would be a non-breaking change to add later.

@pjcozzi What do you think?

pjcozzi · 2016-07-18T18:03:17Z

My gut says to go with 1 until enough people ask for 2

Big +1

…ne into async-to-promises

lilleyse · 2016-07-26T01:58:09Z

Thanks @lasalvavida and @JoshuaStorm

JoshuaStorm added 3 commits July 13, 2016 09:30

Switched to promises, but using anti-patterns. Needs fixing

ce234e8

Merge master

51da324

Fix preemptive resolve issue

0b51b63

JoshuaStorm reviewed Jul 13, 2016
View reviewed changes

JoshuaStorm added 2 commits July 14, 2016 11:37

Fix most promise issues. One issue left. Not ready for review

bd2f5b6

Missed a couple minor fixes

9e62453

JoshuaStorm reviewed Jul 14, 2016
View reviewed changes

lasalvavida added 3 commits July 14, 2016 12:31

Merge branch 'master' of github.com:AnalyticalGraphicsInc/gltf-pipeli…

375180f

…ne into async-to-promises

Fix promises and other cleanup

378c45e

fix jsHint errors

296849e

Fix jsHint (?)

15b6a86

JoshuaStorm added 2 commits July 14, 2016 15:20

Resolve Travis Failure

4f97452

Delete commented line and whitespace

eb3d12e

JoshuaStorm added 2 commits July 14, 2016 15:58

Only promisify individual functions

34f11a0

Cleanup

6aaa0f2

JoshuaStorm changed the title ~~Async to promises. writeGltf, writeBinaryGltf, writeFile -- callbacks -> Promises~~ Async to promises. writeGltf, writeBinaryGltf, writeFile -- callbacks to Promises Jul 14, 2016

More cleanup

9050252

lilleyse reviewed Jul 15, 2016
View reviewed changes

Address comments

86c468b

JoshuaStorm added 3 commits July 15, 2016 15:09

expect promises to resolve

5a155ae

Merge branch 'master' into async-to-promises

f1c468f

Correct AO spec

ab36432

lasalvavida reviewed Jul 15, 2016
View reviewed changes

Address final comments

0a2af08

Merge conflicts

0c4a792

lilleyse mentioned this pull request Jul 20, 2016

Fix write source #146

Merged

lasalvavida added 2 commits July 22, 2016 12:21

Merge branch 'master' of github.com:AnalyticalGraphicsInc/gltf-pipeli…

2965a43

…ne into async-to-promises

Missing excluded _OCCLUSION from bad merge

befafbb

lilleyse merged commit 41c90ae into master Jul 26, 2016

lilleyse deleted the async-to-promises branch July 26, 2016 01:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async to promises. writeGltf, writeBinaryGltf, writeFile -- callbacks to Promises #135

Async to promises. writeGltf, writeBinaryGltf, writeFile -- callbacks to Promises #135

JoshuaStorm commented Jul 13, 2016 •

edited

Loading

JoshuaStorm Jul 13, 2016

lasalvavida Jul 13, 2016

JoshuaStorm Jul 13, 2016

JoshuaStorm Jul 14, 2016

lilleyse Jul 14, 2016

JoshuaStorm Jul 14, 2016

JoshuaStorm Jul 14, 2016

lasalvavida Jul 14, 2016

lasalvavida commented Jul 14, 2016

lasalvavida commented Jul 14, 2016

JoshuaStorm commented Jul 14, 2016

JoshuaStorm commented Jul 14, 2016

lasalvavida commented Jul 14, 2016

JoshuaStorm commented Jul 14, 2016

JoshuaStorm commented Jul 15, 2016

lilleyse Jul 15, 2016

JoshuaStorm Jul 15, 2016

lilleyse Jul 15, 2016

lilleyse Jul 15, 2016

lasalvavida commented Jul 15, 2016 •

edited

Loading

JoshuaStorm commented Jul 15, 2016

lasalvavida commented Jul 15, 2016

lasalvavida Jul 15, 2016 •

edited

Loading

lasalvavida commented Jul 15, 2016

lasalvavida commented Jul 15, 2016

lasalvavida Jul 15, 2016

lasalvavida commented Jul 15, 2016

lasalvavida commented Jul 18, 2016

lilleyse commented Jul 18, 2016 •

edited

Loading

mramato commented Jul 18, 2016

pjcozzi commented Jul 18, 2016

lilleyse commented Jul 26, 2016

		var vertexShaderPath = './specs/data/boxTexturedUnoptimized/CesiumTexturedBoxTest0VS_Binary.glsl';

		describe('getBinaryGltf', function() {

Async to promises. writeGltf, writeBinaryGltf, writeFile -- callbacks to Promises #135

Async to promises. writeGltf, writeBinaryGltf, writeFile -- callbacks to Promises #135

Conversation

JoshuaStorm commented Jul 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasalvavida commented Jul 14, 2016

lasalvavida commented Jul 14, 2016

JoshuaStorm commented Jul 14, 2016

JoshuaStorm commented Jul 14, 2016

lasalvavida commented Jul 14, 2016

JoshuaStorm commented Jul 14, 2016

JoshuaStorm commented Jul 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasalvavida commented Jul 15, 2016 • edited Loading

JoshuaStorm commented Jul 15, 2016

lasalvavida commented Jul 15, 2016

lasalvavida Jul 15, 2016 • edited Loading

Choose a reason for hiding this comment

lasalvavida commented Jul 15, 2016

lasalvavida commented Jul 15, 2016

Choose a reason for hiding this comment

lasalvavida commented Jul 15, 2016

lasalvavida commented Jul 18, 2016

lilleyse commented Jul 18, 2016 • edited Loading

mramato commented Jul 18, 2016

pjcozzi commented Jul 18, 2016

lilleyse commented Jul 26, 2016

JoshuaStorm commented Jul 13, 2016 •

edited

Loading

lasalvavida commented Jul 15, 2016 •

edited

Loading

lasalvavida Jul 15, 2016 •

edited

Loading

lilleyse commented Jul 18, 2016 •

edited

Loading