Overloads should be replaced with generics.

[saul-jb]

(Just logging a quick issue in case I don't get to a PR) Some of the methods use weird overloads instead of generics and then call generics, this can make it super painful to make code that wraps these methods with the right types without type errors.

The quintessential example is in the generate code:

ollama-js/src/browser.ts

Lines 109 to 121 in dfc7988

	generate(
	request: GenerateRequest & { stream: true },
	): Promise<AsyncGenerator<GenerateResponse>>
	generate(request: GenerateRequest & { stream?: false }): Promise<GenerateResponse>
	async generate(
	request: GenerateRequest,
	): Promise<GenerateResponse | AsyncGenerator<GenerateResponse>> {
	if (request.images) {
	request.images = await Promise.all(request.images.map(this.encodeImage.bind(this)))
	}
	return this.processStreamableRequest<GenerateResponse>('generate', request)
	}

The first issue is this makes the stream parameter un-assignable to boolean, meaning this code has a type error:

const generate = (request: GenerateRequest, stream: boolean) => {
  return ollama.generate({ ...request, stream })
}

This is due to ambiguity in which overload signature should be used.

Things get even worse if you want to make stream generic so the right return type can be passed - you have to do different calls for the different generic cases and cast the result to a type conditional.

The overloading works well for simple use cases but generics shouldn't make a difference for the simple uses while making types much easier to work with on more complex ones.

[Pixelycia]

public generate<T extends GenerateRequest>(request: T): Promise<T extends { stream: true } ? AsyncGenerator<GenerateResponse> : GenerateResponse> { ... }

[saul-jb]

Just been looking at this again and it is not as straight-forward as I had assumed (using conditionals like @Pixelycia mentioned).

You can do it through conditionals but you have to type cast from the specific case to the conditional case every time you return from a method which becomes quite messy.

Another way to get types to behave correctly in most cases is to add a ton of signatures:

  generate(
    request: GenerateRequest & { stream: true },
  ): Promise<AsyncGenerator<GenerateResponse>>
  generate(request: GenerateRequest & { stream: false }): Promise<GenerateResponse>
  generate(request: GenerateRequest & { stream: boolean }): Promise<GenerateResponse | AsyncGenerator<GenerateResponse>>
  generate(request: GenerateRequest): Promise<GenerateResponse>
  async generate(
    request: GenerateRequest & { stream?: boolean },
  ): Promise<GenerateResponse | AsyncGenerator<GenerateResponse>> { ... }

import ollama from './index'

const req = { prompt: '', model: ''}
const stream = Math.random() > 0.5

const a =  ollama.generate({ ...req, stream: true })
const b =  ollama.generate({ ...req, stream: false })
const c = ollama.generate({ ...req, stream: undefined })
const d = ollama.generate(req)
const e = ollama.generate({ ...req, stream })

const test = <S extends { stream?: boolean }>(stream: S) => {
 const f = ollama.generate({ ...req, ...stream })
}

This behaves correctly in cases a-e but fails in case f.

Both adding more signatures or lots of ugly type conditional casts is messy, could we consider splitting the stream cases into separate methods?

If we did something like ollama.stream.generate, we could get types to work in all cases and get rid of that weird return type: Promise<AsyncGenerator<GenerateResponse>>.

Another possibility is to make streaming the default case and export a helper to convert the stream to a single value like:

ollama.generate({...}) // stream: true
await parseStream(ollama.generate({...})) // stream: false

What do you think @BruceMacD?