jupyterlite / jupyterlite

Wasm powered Jupyter running in the browser 💡
https://jupyterlite.rtfd.io/en/stable/try/lab
BSD 3-Clause "New" or "Revised" License
3.92k stars 313 forks source link

Breaking Basekernel.executeRequest API change suggestion #1504

Open MRYingLEE opened 1 month ago

MRYingLEE commented 1 month ago

Problem

In BaseKernel of Jupyterlite:

https://github.com/jupyterlite/jupyterlite/blob/bcc7ac780bb49914c7130e76bef12ad0b7db8432/packages/kernel/src/kernel.ts#L159-L161

While in the corresponding part of Jupyterlab:

https://github.com/jupyterlab/jupyterlab/blob/48691c6a3040af46c4ed084a06c2d83c2d9e9236/packages/services/src/kernel/default.ts#L705-L709

  requestExecute(
    content: KernelMessage.IExecuteRequestMsg['content'],
    disposeOnDone: boolean = true,
    metadata?: JSONObject
  )

So the metadata parameter is missed in calling. Sometimes we do need to transfer some settings from frontend to backend. So it is better to have metadata as a parameter.

Proposed Solution

https://github.com/MRYingLEE/jupyterlite/blob/df14e5d4fe9305cacc91b16ec1b74a1ed29a904a/packages/kernel/src/kernel.ts#L160-L163

  abstract executeRequest(
    content: KernelMessage.IExecuteRequestMsg['content'],
    metadata?: JSONObject
  ): Promise<KernelMessage.IExecuteReplyMsg['content']>;

And also: https://github.com/MRYingLEE/jupyterlite/blob/df14e5d4fe9305cacc91b16ec1b74a1ed29a904a/packages/kernel/src/kernel.ts#L559

const reply = await this.executeRequest(executeMsg.content, executeMsg.metadata);

instead of https://github.com/jupyterlite/jupyterlite/blob/bcc7ac780bb49914c7130e76bef12ad0b7db8432/packages/kernel/src/kernel.ts#L557C5-L557C65

const reply = await this.executeRequest(executeMsg.content);

And

https://github.com/MRYingLEE/jupyterlite/blob/70752738aa85dd989a23383aa9cb8082ab0b6f08/packages/kernel/src/tokens.ts#L173-L177

  execute(
    content: KernelMessage.IExecuteRequestMsg['content'],
    parent: any,
    metadata?: JSONObject,
  ): Promise<KernelMessage.IExecuteReplyMsg['content']>;

instead of https://github.com/jupyterlite/jupyterlite/blob/bcc7ac780bb49914c7130e76bef12ad0b7db8432/packages/kernel/src/tokens.ts#L172C1-L175C57

  execute(
    content: KernelMessage.IExecuteRequestMsg['content'],
    parent: any,
  ): Promise<KernelMessage.IExecuteReplyMsg['content']>;

Additional context

This is a breaking change. So all the kernels have to update the API also.

For your reference.