support for a status callback

[danmarshall]

Hello,
I just wanted to get your thoughts on support for a status callback. This can allow scenarios which can inform what the worker is doing - such as updating a percent complete.

I've only added it to STLASCII, but if you like this idea I can do it to each format.

Serializers:

• amf-serializer
• dxf-serializer
• json-serializer
• stl-serializer A
• stl-serializer B
• svg-serializer
• x3d-serializer

Deserializers:

• amf-deserializer
• gcode-deserializer
• json-deserializer
• obj-deserializer
• stl-deserializer
• svg-deserializer

[kaosat-dev]

Hi @danmarshall !
Thanks for the addition !
I like the intent, it can be very useful ! two things though :

• I would check for the presence of the callback instead of having a no-op function, since a no-op would still be called for every polygon, and result in a possible slowdown
• this one is less important and I might be too finicky , so feel free to disregard :) is percentage the best or should it be progress ? also 0 - 100 or 0-100 as float ? :)

What is your take on this @z3dev ?

[z3dev]

@danmarshall I believe that another such request has been made for the website, so obviously such a call back would be necessary. I think this would be the JavaScript way to implement such functionality.

Some formats provide an indication of 'size' so a progress would be possible. For STL, I would guess that file size could be used to approximate the number of faces.

[danmarshall]

Thanks @kaosat-dev & @z3dev ! I renamed the property to progress. And not calling a no-op.
Take another look, and I can add it to the others.

Should I also add it to de-serializers?

[z3dev]

@danmarshall Please make the changes to both serializers and deserializers. Let us know if you have any questions.

[danmarshall]

OK, this is code complete. Some of the deserializers which parse xml are event based, so we don't really know the percentage complete. But at least we have 0% - 50% - 100% for everything minimally.

I wasn't able to run the ava test suite. Is there another way to test this?

[danmarshall]

@kaosat-dev @z3dev can you take a look? :)

[kaosat-dev]

Thanks for all the additions @danmarshall !
It looks ok as far as I am concerned.
However I think some basic unit testing would be good :)

I wasn't able to run the ava test suite

Did you try npm t in the root folder (after npm install) it should setup & test all the subpackages.

[z3dev]

Looks fine.

@danmarshall thanks for the quick turnaround!

[danmarshall]

On a separate, clean clone, I get an error running the tests:

lerna ERR! test Errored while running script in '@jscad/dxf-serializer'
lerna ERR! execute Error: Command failed: npm run test
lerna ERR! execute
lerna ERR! execute   × Couldn't find any files to test

[danmarshall]

@kaosat-dev @z3dev it seems like we need a test for the dxf serializer first.

[z3dev]

Don't bother now, as I'm working on the deserializer at the moment, and will revisit the serialzer next.

[kaosat-dev]

@danmarshall as @z3dev do not worry about the dxf serializer , but please add some very simple tests for the callback system for those who already have some unit tests (I think in most cases you can just copy / paste tests, and just check if the callback is fired correctly). Then we can merge :)

Btw in ava callback based tests have a slightly different syntax:

test.cb(t => {
	t.plan(1) // how many checks

	someAsyncFunction(() => {
		t.pass()
		t.end() // will not work without this
	});
})

[danmarshall]

Hi guys, I'm not able to get any of the ava tests to run. I don't think it's specific to the dxf-serializer project. Even if I go into the amf-serializer project, I get:

× Couldn't find any files to test

of course, I first needed to npm install ava --save-dev in the amf-serializer project.

Can you try a clean install and see if any of the tests works for you?

[kaosat-dev]

@danmarshall are you getting the same errors when running npm t from the root folder of IO ?

[kaosat-dev]

from

• a fresh clone
• npm i at the root level (sorry to insist, it matter a lot :) )
• npm t at the root level shows me this (ie only one lacking tests and not really an error in the end)

[kaosat-dev]

Basically since all the repos are interconnected, lerna deals with 'bootstrapping' the common dependencies and linking them together.
If you run tests at each package level , you loose some of those advantages and need to install stuff manually.
I am kinda baffled by ava not finding tests though.

[danmarshall]

(Yet another) fresh clone - I get the same result as I did above.
I'm on Windows btw.

[kaosat-dev]

damn, sorry @danmarshall, I have no Windows machine to test it on , and the ava testing paths are simple enough './test.j' ...
in that case, exceptionally, I guess we will merge without the tests & I will have to add them after the fact.
Or if you have a tiny bit of time left, could you add a few tests 'blindly' (without actually being able to run them I know it is not very practical, so I can understand if you do not feel like it).

[danmarshall]

Or, you can clone my fork, and run the tests on your machine. That would give us both a better sense of confidence :)

[kaosat-dev]

haha yes that is a good option indeed ;) but let us not forget Travis CI seems to think it is all good so far, as all tests are passing there !

[danmarshall]

Seems like the problem might be between lerna and ava. Since lerna is the manager, and there seems to be an open issue in ava about cwd.

[danmarshall]

ok, added a test for progress status callback 🤠

[kaosat-dev]

Ok will merge & release and add the rest of the tests.
Thanks for the addition @danmarshall !
Good luck with the rest of the updates for Maker.js ;)

[kaosat-dev]

btw @danmarshall are you planning of using the modules in this repo directly or the openjscad package ?

[danmarshall]

@kaosat-dev thanks!! I am planning on using the individual modules directly. I also may use the UI Viewer alone without the Processor.