Community review

[JayPanoz]

This branch should make it easier for the community to review.

[mickael-menu]

Bear with me, because I don't know much about TypeScript, but why use an interface in this case? Format will have only one implementation.

I'm also a bit confused how Format is created in its constructor, why not constructor(name: string, mediaType: MediaType, fileExtension: string)?

[JayPanoz]

It’s more like I wanted to keep named parameters in static methods for more clarity as this can turn super ugly when done directly in constructor, cf. this SO question.

[mickael-menu]

@danielweck During the call there was a consensus in favor of using interfaces to create anonymous object arguments to replace the concept of "named parameters". But we're not sure about the naming of such interfaces. Is there a standard convention for this? Or a more idiomatic way to tackle the issue of named parameters?

[mickael-menu]

How about using a type alias on the object structure instead of an interface? This doesn't solve the naming issue, but at least it's clear that it's not a type to be implemented.

type FormatDefinition = { name: string }

class Format {
  public name: string;

  constructor(params: FormatDefinition) {
    this.name = params.name;
  };

}

A few other ideas for the name: FormatDefinition, FormatConstructor, FormatSchema, FormatData and FormatLike, as seen in this PR.

Technically we could also ditch the name entirely. But I don't know enough TypeScript to see if that could be a problem:

class Format {
  public name: string;

  constructor(params: { name: string }) {
    this.name = params.name;
  };

}

[danielweck]

I have come across a number of Javascript / TypeScript APIs where objects are used as function parameters in order to pass multiple atomic pieces of data (quick example from the top of my head: React component props and state), but there are many instances of the opposite design pattern (extreme example: curry-ing which “spreads” individual arguments over multiple function calls).

I think that generalising the use of objects to carry multiple function arguments is an unusual approach, but as far as I know it is not an anti-pattern ... though I am sure quite a few JS/TS programmers will find this “weird”, so I am not convinced this will improve the developer experience (DX) across the board.

Note that there is a runtime cost incurred when creating objects instead of just spreading the function arguments the “normal” way. Garbage collection in JS engines is pretty good, and the Readium SDK is not a “real time” / “high performance” system ... but still, this is something to keep in mind in addition to DX considerations.

Another potential problem is that variable with primitive types passed as function arguments are sometimes not used as constants within the closure scope, and instead are modified directly in order to avoid creating unnecessary stack allocations (i.e. this is an implementation detail below the API surface). With objects, there is the potential problem of mutating properties in one function scope, and inadvertently changing the value in another. Just a thought.

[mickael-menu]

Thanks for chiming in @danielweck. Yeah that would probably be overkill. If I understood correctly, @JayPanoz planned to use these mostly for large constructors with multiple parameters of the same type. One could argue that Format doesn't fit the bill, but it would probably be safer for Link, Metadata, etc. Because adding a new parameter constructors in an update means that the constructor could break silently if the type of the new parameter is the same.

[danielweck]

Yes, it is quite common in JavaScript/TypeScript APIs to use a "config" object argument which properties are assigned defaults when not defined on the caller's side. I can see how this would work for constructors or other initializers. That being said, it is also quite common to have such "config" function argument alongside other arguments, so the design pattern is quite open.

Personally I would favour consistency, so if "object with named properties is used to pass function arguments" gets applied to constructors, then I would prefer the design pattern to be used for all other public API functions (which then becomes quite cumbersome, as noted in a previous comment).

[mickael-menu]

This part is quite complicated, so I think it would be important to add some unit tests once we have the test suite set up, based on Swift for example.

Also for the comparison functions below.

[mickael-menu]

matches() is not just an alias to contains(), but needs to work in both directions. So it should be something like this:

return this.contains(other) || other.contains(this)

But I think the focus should be on implementing unit tests, they should catch that kind of errors.

[JayPanoz]

Note: Resolve Pending as I wanted to doublecheck.

[mickael-menu]

Is there a finite amount of contains properties? On mobile we used a simple string array, but an enum might be better if we're sure it's all there is.

[JayPanoz]

EPUB Spec

WebPub Manifest

On a related note, shouldn’t the js value be scripted?

[jccr]

In TS string literal unions are an alternative to enums.

I prefer them as they are more lightweight in terms of writing them and with the final output (no runtime artifacts)

[mickael-menu]

Looks like this is an exhaustive type indeed, let's confirm that on the call with Hadrien and probably we can migrate to enums on mobile.

[JayPanoz]

@jccr Would you consequently prefer ReadingProgression and EPUBLayout as string literal unions too? Let’s try to keep it consistent.