opendesigndev / octopus

Monorepository for Octopus 3+ converters and related tools
Apache License 2.0
4 stars 7 forks source link

[WORK in PROGRESS]: common interface for octopus converters #97

Open alexspevak opened 1 year ago

alexspevak commented 1 year ago

In this document, I will be describing the current status and future steps which we have been or will be regularly discussing with @cerosnikita on how to unify all octopus-3 interface.

At this moment readers are described and this will be one of my first steps.

READERS


Constructor Options

Node

Readers will have following options: {path: string}

Notes

export type WithRendererOptions = { /* Path to the .psd design file. / path: string

/* Unique ID. If not passed, will be generated by UUIDv4 / designId?: string }

`designId` will be removed and `renderer` as optional parameter will stay as it is necessary for better image quality. 

- octopus-ai already uses such interface

### WEB
Web will use  `{file: Uint8Array}` option. 
#### Notes
- octopus-xd already uses  `{file: Uint8Array}` option
- octopus-fig will not be affected as described in Node section
- octopus-psd uses same options as described in Node section. `path` will be replaced with `{file: Uint8Array}` and designId removed
- octopus-ai uses same interface as in `Node` and will need refactoring of params and converting examples. 

## Storing on disk
Optionally, there are some internal optimizations which use file system to store assets, but that's not a general rule, so some readers still use only memory data:
- octopus-xd reader has constructor option `storeAssetsOnFs` and it will assets on file system if such option is provided
-  octopus-ai by default stores assets on file system because external dependency `import { FSContext } from '@opendesign/illustrator-parser-pdfcpu/fs_context'` stores on disk. 

## Public interface
 We will have the following public interface:
 - fileMeta() -> this will return ids of top level artboards. octopus-fig and octopus-psd already have such public method but as `getFileMeta` and `readFileMeta`. Other converters do not have such method yet. ReturnValue for this is:
```ts
type NamedArtboard = {
  id: number
  name: string
}

type FileMeta = {
  designName: string
  content: {
    topLevelArtboards: NamedArtboard[]
  }
}

Notes

Images

Image data should be read as getImageData function which returns Promise<Uint8Array>.

Notes

Exporters

Images

We have decided that images will exported as type {id:string, getImageData:()=>Promise<Uint8Array>, path?:string}. User will have option to process images in exporter. This is specifically important for web version of octopus-ai where processing images is very expensive. At the moment all of the converters pass processed images to converters, some of them just have intermediate step.

Notes

Differences in individual converters:

OCTOPUS-COMPONENT

I propose to use following interface:

  (alias) type GenericComponentConversionResult<T extends object> = {
    id: string;
    value: T | null;
    error: Error | null;
    time: number;
}
  export type ComponentConversionResult = GenericComponentConversionResult<Octopus['OctopusComponent']>;

   async exportComponent(
    result: ComponentConversionResult,
    role?: Manifest['Component']['role']
  ): Promise<unknown | null> 

GenericComponentConversionResult is type shared from octopus-common (already implemented in work in progress merge request). This method should be mandatory.

Notes

  1. octopus-psd uses similar interface where role is missing. As this is optional parameter, this can be easily adjusted.
  2. octopus-xd and octopus-ai use exportArtboard where they export Octopus['OctopusComponent'] but also SourceComponent. SourceComponent should be exported in separate method.
  3. octopus-fig is already using proposed interface

MANIFEST

For exporting Manifest['OctopusManifest'] I would propose following interface:

type GenericDesignConversionResult<T extends object> = {
    manifest: T;
    time: number;
}
export type DesignConversionResult = GenericDesignConversionResult<Manifest['OctopusManifest']>
exportManifest(designConversionResult: DesignConversionResult): Promise<unkown>

GenericComponentConversionResult is type shared from octopus-common (already implemented in work in progress merge request). This method should be mandatory.

Notes

  1. octopus-fig is plainly exporting Manifest['OctopusManifest'] and it does not convert it with benchmark function, therefore DesignConversionResult is not appliable. We can convert with benchmark function in octopus-fig too and have same interface.
  2. All other converters implement this interface.

signaling that last item has been sent for exporting

For this function

finalizeExport(): void

is implemented in all converters and needs no change. This method should be mandatory in proposed interface.

statistics

I propose following method:

exportStatistics(statistics?: object): Promise<unknown>

This is only implemented in octopus-psd and should be implemented in all other exporters. statistics param is optional as not always user will be collecting statistics. This method should be mandatory in proposed interface.

path to target directory

We use following method in all converters except for octopus-fig:

getBasePath(): Promise<unknown>

octopus-fig is missing this method but can be easily implemented. WebExporter can implement empty mock method.

SourceArtboard

octopus-xd and octopus-ai export its SourceArtboard in the same method they export Octopus['OctopusComponent']. This should be separated and new method should be optional in interface as it is used for dev purposes. I propose following interface:

  async exportSourceArtboard(artboard: SourceArtbord): Promise<unknown> 

SourceDesign

only octopus-xd exports its Sourcedesign with following signature

 type SourceResources = { manifest: string; interactions: string; resources: string }

  async exportSourceDesign(design: SourceDesign): Promise<SourceResources> 

I propose not to have interface. octopus-xd user can implement this without interface.

SPECIFIC METHODS:

Following methods are specific individual converters, they can be left as they are and no interface is needed.

octopus-fig:

  1. async exportRawDesign(raw: RawDesign): Promise<string>
  2. async exportRawComponent(raw: RawLayerContainer, name: string): Promise<string>
  3. async exportRawChunk(raw: ResolvedStyle, name: string): Promise<string> 

    octopus-ai:

    octopus-ai uses :

 exportAuxiliaryData(_design: SourceDesign): Promise<AuxiliaryData>

to export metaData about the Adobe Illustrator version and private text data.

Naming:

I propose following names for exporters : WebExporter, LocalExporter and DebugExporter.

cerosnikita commented 1 year ago

designId will be removed

So, it's nothing necessary? What did we use it for?

alexspevak commented 1 year ago

designId will be removed

So, it's nothing necessary? What did we use it for?

We used it for designating design id if provided, instead of random, but this was only used in octopus-psd

cerosnikita commented 1 year ago

We used it for designating design id if provided, instead of random, but this was only used in octopus-psd

That's obvious. But why for?


Another question is regarding fileMeta. I'd name it designMeta so we don't need to bring on new terminology (such as file is). Also, I am not sure about type:

{
  designName: string
  content: {
    topLevelArtboards: NamedArtboard[]
  }
}

I think in Figma we provide not only top level artboards, but also components. Also, there's no NamedArtboard type in your examples.

Update: I have found the section when you say that Figma has richer result type and that it's Figma specific. It's not Figma specific and it should be done in other converters too.


Regarding cleanup(). I was thinking about it and actually I think that this shouldn't be generalized because many readers will not have nothing to clean up. Also, by standardizing the API we only define exact subset of possible API, but not the complete API because Readers should just implement our interface, but there's nothing wrong if Readers will extend this API. So, cleanup() is just a case when we extend Reader and we don't have to use it necessary on every reader.


Regarding getSourceDesign(). I think we should expect that in the future we will want to use more options, so instead of passing ids as argument I'd rather use an object with named property ids.

I think that getSourceDesign() is better name for this method than parse() because parse() is too abstract.


Regarding images. I see that you suggested the following type:

{
  id: string,
  getImageData: () => Promise<Uint8Array>, 
  path?: string
}

I am not sure about that path. Why do you think we need it? Also, I'd rename getImageData to just getImage.

cerosnikita commented 1 year ago

Also, maybe you remember that in some Exporters which you used for testing you had to implement methods with the body like:

exportSomething() {
  console.log('exporting something')
  return Promise.resolve()
}

This should be handled by some abstract class which by default implements all the methods for exporters/readers and just use some simple dumb logic inside of such methods. For example:

class AbstractExporter {
  exportSomething() {
    console.warn('Exporing something (warning - default (not overriden) method is called!)')
    return Promise.resolve(null)
  }
}

class MyExporter extends AbstractExporter {
  exportSomething() {
    // ... some real logic
  }
} 

I am not sure what we should use here, actually - implements or extends. Because: implements will force user to implement all the methods of exporter - I am not sure it's developer friendly. On the other hand it's more correct.

alexspevak commented 1 year ago

Also, maybe you remember that in some Exporters which you used for testing you had to implement methods with the body like:

exportSomething() {
  console.log('exporting something')
  return Promise.resolve()
}

This should be handled by some abstract class which by default implements all the methods for exporters/readers and just use some simple dumb logic inside of such methods. For example:

class AbstractExporter {
  exportSomething() {
    console.warn('Exporing something (warning - default (not overriden) method is called!)')
    return Promise.resolve(null)
  }
}

class MyExporter extends AbstractExporter {
  exportSomething() {
    // ... some real logic
  }
} 

I am not sure what we should use here, actually - implements or extends. Because: implements will force user to implement all the methods of exporter - I am not sure it's developer friendly. On the other hand it's more correct.

you can force developer to create something by using abstract keyword before the method

cerosnikita commented 1 year ago

you can force developer to create something by using abstract keyword before the method

I know, but I am saying that I am not sure that forcing is a dev-friendly practice. Actually, I think implements is better idea. Atleast user will implement all the required methods of exporter. 🤔

alexspevak commented 1 year ago

I am confused. Do we want to be dev-friendly or we want to force user to implement something? With abstract class we can achieve both things. With implements we can only force them to implement required methods.