-
Notifications
You must be signed in to change notification settings - Fork 276
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow providing your own ContentsAPI implementation for the emscripten DriveFS #1383
Conversation
Converting into draft. I want to make extra testing on this before continuing. |
It looks like making backward incompatible changes to the DriveFS will break the UI-tests here because it's using pyodide-kernel which builds against a specific DriveFS version. It's fine in this PR because I'd like it to be backward compatible, but it may be annoying some day if we want to make intentional breaking changes. |
This is ready for review. The UI-tests failure appear on the main branch too, fixing it in #1390 |
CI is passing now @jtpio |
): Promise<TDriveResponse<T>> { | ||
switch (request.method) { | ||
case 'readdir': | ||
return this.readdir(request as TDriveRequest<'readdir'>) as Promise< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand we have no choice to cast here. It seems you're casting to any
in https://github.com/jupyter-lsp/jupyterlab-lsp/pull/278/files#diff-7c7804a4b99d4b67b07ab518d033a6de85bdbf291e5e03400106901ff2501d5aR30.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From outside calls of processDriveRequest
, the response type is correctly computed.
@bollwyvl please tell me what you think! |
Friendly ping! |
Since "contents" in our API language is typically plural, I would suggest renaming |
I just rebased (and squashed to make my life easier during the rebase). Thanks @afshin I resolved it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Let's get this in and released in the next alpha, so it can be tested downstream.
@martinRenou @bollwyvl please feel free to open a new issue or PR if something needs to be changed or fixed, thanks!
@martinRenou, could you help me make sense of this new feature? Following a quick search, I was unable to find documentation on jupyterlite/packages/contents/src/drivefs.ts Line 375 in a9dc320
For context: I am still looking for a good way to use |
This PR allows jupyterlite/xeus#87.
ContentsAPI
implementation in the DriveFSbroadcast.ts
which processes the driveFS request, so that it's easily reusable in another implementation