Skip to content
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

Merged
merged 4 commits into from
Jun 3, 2024

Conversation

martinRenou
Copy link
Member

@martinRenou martinRenou commented May 16, 2024

This PR allows jupyterlite/xeus#87.

  • Allow easily overwriting the ContentsAPI implementation in the DriveFS
  • Extract the logic from broadcast.ts which processes the driveFS request, so that it's easily reusable in another implementation

Copy link
Contributor

lite-badge 👈 Try it on ReadTheDocs

@martinRenou martinRenou changed the title FileSystem calls over Atomics.wait instead of service worker, when available Allow providing your own ContentsAPI implementation for the emsciprten DriveFS May 17, 2024
@martinRenou martinRenou changed the title Allow providing your own ContentsAPI implementation for the emsciprten DriveFS Allow providing your own ContentsAPI implementation for the emscripten DriveFS May 17, 2024
@martinRenou martinRenou marked this pull request as ready for review May 17, 2024 07:58
@martinRenou martinRenou marked this pull request as draft May 17, 2024 15:07
@martinRenou
Copy link
Member Author

Converting into draft. I want to make extra testing on this before continuing.

@martinRenou
Copy link
Member Author

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.

@martinRenou martinRenou marked this pull request as ready for review May 21, 2024 08:35
@martinRenou
Copy link
Member Author

This is ready for review. The UI-tests failure appear on the main branch too, fixing it in #1390

@martinRenou
Copy link
Member Author

CI is passing now @jtpio

): Promise<TDriveResponse<T>> {
switch (request.method) {
case 'readdir':
return this.readdir(request as TDriveRequest<'readdir'>) as Promise<
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

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.

@martinRenou
Copy link
Member Author

martinRenou commented May 23, 2024

@bollwyvl please tell me what you think!

@jtpio jtpio requested a review from bollwyvl May 28, 2024 08:42
@martinRenou
Copy link
Member Author

Friendly ping!

@afshin
Copy link
Contributor

afshin commented May 31, 2024

Since "contents" in our API language is typically plural, I would suggest renaming drivecontent.ts to drivecontents.ts

@martinRenou
Copy link
Member Author

I just rebased (and squashed to make my life easier during the rebase).

Thanks @afshin I resolved it.

Copy link
Member

@jtpio jtpio left a 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!

@jtpio jtpio merged commit ea95607 into jupyterlite:main Jun 3, 2024
20 checks passed
@martinRenou martinRenou deleted the atomics_wait branch June 3, 2024 14:28
@michaelweinold
Copy link

@martinRenou, could you help me make sense of this new feature?

Following a quick search, I was unable to find documentation on ContentsAPI functionality.
From your changes, I see, that:

this._driveName = driveName;

  1. Would this allow me to chose a different file system as default for my JupyterLite site?
  2. Is this PR at all related to other efforts related to the file system, like:

For context: I am still looking for a good way to use sqlite3 databases in the default directory - which is currently not possible, as per:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants