Fix issue 1365: support binary deserializers in CLI

[eldanb]

All Submissions:

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?
• Does your submission pass tests?

Note: please do NOT include build files (those generate by the build-xxx commands) with your PR,

Thank you for your help in advance, much appreciated !

[z3dev]

@eldanb Oh! You provided the changes. Nice!

The changes are fine except for one point. Each deserializer has a specific API, and changing those now in V2 is probably not a good idea. There are other applications using those deserializers. :(

I suggest doing something like the core library does, and choose the buffer contents based on file extension.

OpenJSCAD.org/packages/core/src/web/walkFileTree.js

Line 53 in 15a50a0

if (binaryMimetypes[ext]) {

Also, there are some new formats coming in V3, including 3MF. So, maybe the IO library needs another list, or extend the list of 'formats'. Then there's one place that keeps track of the required 'binary' versus 'string' contents.

[eldanb]

But I didn't change the serializers' APIs.
Their API could always receive either an array buffer or a string (that's what happens in walkFiles). Some of them would fail when they get an array buffer. That's what I fixed.

To be clear -- anything that worked with the deserializers prior to my change would continue working with them.
Things that previously failed may now succeed (correctly)

[eldanb]

Looking at binaryMimeTypes like this means that there are multiple places that encode the deserializer's expected API, which I recommend to avoid.

If you prefer we can decide that a deserializer function carries a "isBinary" property, and based on that we either load binary or text. But that too is more cumbersome, I believe, than simply declaring the deserializer's API (as it is now) as receiving "string or ArrayBuffer"

@z3dev

[z3dev]

@eldanb a few questions and suggestions.

[eldanb]

@z3dev I've committed changes per your comments.
Note that I've not advanced any of the package's versions, I'm assuming this is something you do as part of your release/publish routine?

[z3dev]

@z3dev I've committed changes per your comments.
Note that I've not advanced any of the package's versions, I'm assuming this is something you do as part of your release/publish routine?

Yeah. The version updates happen automatically via Lerna.

I'll double check.

[z3dev]

@platypii please review

[platypii]

The code looks fine, and I appreciate the tests.

However I have to admit I was unable to re-pro the issue? I downloaded example_project.zip from your issue #1365. But when I use the latest published cli, I don't get an error? Maybe I'm missing something?

(update: nevermind I see that I needed to run it on the whole folder to get the error)

[eldanb]

To reproduce the issue you should require() the stl. There's a sample project there which I've added to the tests. Running the CLI on it fails. also the tests I added for back-loading projects fail without the fix.

[platypii]

Confirmed I was able to reproduce the failure on master, but success on this pr branch 👍

[z3dev]

@eldanb these changes are all set to go.

fyi, there are coding style standards. But there are probably lots of exceptions in V2.

npm run lint

i will roll a release this weekend.