Protocol feedback

[johnfairh]

I had a go at implementing the host side of this in Swift. It came out OK -- I found the docs really clear and well written. Here’s some feedback on the protocol.

Binary delimiter. The protobuf docs are a bit vague on exactly how to delimit streamed messages. The implementations I’ve looked at (C Java Swift) all use a Varint32. The Sass protocol docs are clear that it uses a fixed-width uint32 so I was never confused or misled, but it may be worth changing the protocol to use Varint32 to make it easier to adopt using the standard protobuf helpers.

Version. I’d find a simple ‘give me some version information’ compiler request useful for problem characterization/debugging. In particular the Swift package infrastructure isn’t sophisticated enough to be able to declare a dependency on a Dart binary so I’m reliant on my user to assemble the right pieces: being able to check (or at least log) that the given compiler is version X of implementation Y would be useful.

CompileReq.string.importer. This importer is ignored in the code -- is that a todo or is the protobuf field a leftover?

[I had some more here about FileImporters and invoking CompilerFunctions but then I found the closed issues, so, never mind!]

[nex3]

Thanks for the feedback, John!

Binary delimiter. The protobuf docs are a bit vague on exactly how to delimit streamed messages. The implementations I’ve looked at (C Java Swift) all use a Varint32. The Sass protocol docs are clear that it uses a fixed-width uint32 so I was never confused or misled, but it may be worth changing the protocol to use Varint32 to make it easier to adopt using the standard protobuf helpers.

The difficulty here is that, for languages that don't have built-in helpers for delimited messages in streams (which I believe includes both Dart and JS), implementing a var by hand is more complicated than implementing a uint32. That said, the current protocol does inherently limit us to returning at most 4.3 gigabytes of Sass, which could be a problem. I think fixing this is probably worth the extra complexity.

Version. I’d find a simple ‘give me some version information’ compiler request useful for problem characterization/debugging. In particular the Swift package infrastructure isn’t sophisticated enough to be able to declare a dependency on a Dart binary so I’m reliant on my user to assemble the right pieces: being able to check (or at least log) that the given compiler is version X of implementation Y would be useful.

For now, we recommend bundling the embedded executable with your package rather than requiring users to download it separately, since there's only one embedded compiler that exists. That said, it's still probably a good idea to have a way to learn what version of the compiler you're interacting with for future-proofing purposes. Let's follow up in #14.

CompileReq.string.importer. This importer is ignored in the code -- is that a todo or is the protobuf field a leftover?

That's an oversight, I'll fix it.

[johnfairh]

These all sound like good changes to me. I will have a think about how best to bundle a compiler binary with the package, at least for the common platforms -- thanks for the prompt, will definitely be more consumable that way.

Looks like everything is covered so I'll close this now.

[nex3]

I'm going to re-open until we've actually landed the fixes in question.

[nex3]

All right, I think all of these are fixed now. Thanks for the report!