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

Switch to use browser-fs-access npm module directly #8673

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tomayac
Copy link

@tomayac tomayac commented Feb 20, 2023

This PR switches logseq to use the browser-fs-access npm module directly. It was using code from the project already, and the mentioned babel transform issues have (hopefully) been fixed.

@CLAassistant
Copy link

CLAassistant commented Feb 20, 2023

CLA assistant check
All committers have signed the CLA.

@logseq-cldwalker
Copy link
Collaborator

@tomayac Hi. Thanks for the interest. Have you been able to QA what this is being used for? If we decide to go ahead with this, we'll probably ask for some QA since we don't have tests here

@tiensonqin Thoughts on if this worth doing at this time? I see this is related to code introduced back in #724

@tomayac
Copy link
Author

tomayac commented Feb 20, 2023

This is barely a housekeeping PR that doesn’t introduce new functionality, but simply uses the browser-fs-access library code directly as a dependency; the same code which #724 extracted from the library manually (as per the comments in the logseq source code and the PR), since the build tooling used back then wasn’t compatible with logseq’s, but which after updating works fine now.

@Bad3r
Copy link
Collaborator

Bad3r commented Feb 20, 2023

@tomayac Hi. Thanks for the interest. Have you been able to QA what this is being used for? If we decide to go ahead with this, we'll probably ask for some QA since we don't have tests here

@tiensonqin Thoughts on if this worth doing at this time? I see this is related to code introduced back in #724

This is what the web app uses to access the local filesystem.

https://developer.mozilla.org/en-US/docs/Web/API/File_System_Access_API

I will build the web app from this branch and release it to the testing channel.

I'll provide the link once it's done building.

@Bad3r Bad3r self-requested a review February 20, 2023 21:37
@tomayac
Copy link
Author

tomayac commented Feb 20, 2023

(FWIW, I successfully tested locally in the Web browser and in Electron.)

@Bad3r
Copy link
Collaborator

Bad3r commented Feb 20, 2023

(FWIW, I successfully tested locally in the Web browser and in Electron.)

Thank you. Its always good to have an extra pair of 👀

Docker image is building: https://github.com/logseq/logseq/actions/runs/4227294573/jobs/7341620379

once done it will be available via the testing channel:
https://github.com/logseq/logseq/pkgs/container/logseq-webapp

@Bad3r
Copy link
Collaborator

Bad3r commented Feb 21, 2023

Note
Non-blocking
Effacts the latest release as well

hmm something is wrong. I can't get the filesystem API to work.
Tested in a clean testing env:

  • Windows version 10.0.19044 Build 19044
  • Chrome: Version 110.0.5481.104 (Official Build) (64-bit)
  • Edge: Version 110.0.1587.50 (Official build) (64-bit)

image

Not sure if this issue was introduced by this PR. I remember someone reporting a similar issue on the forum or discord. More testing is needed.

Anyone is able to test the docker image on MacOS?

docker run -d --rm -p 3001:80 ghcr.io/logseq/logseq-webapp:testing

then navigate to http://0.0.0.0:3001 to view the webapp.

CC: @andelf, @cnrpman

@Bad3r
Copy link
Collaborator

Bad3r commented Feb 21, 2023

Tested and the issue also affects the latest release 🤦‍♂️
That means the issue is non-blocking for this PR.
Not sure what broke it.

docker run -d --rm -p 3001:80 ghcr.io/logseq/logseq-webapp:latest

@tomayac
Copy link
Author

tomayac commented Feb 21, 2023

Glad to learn that the breakage is unrelated to my PR, but curious to understand what caused it in the latest release. I’ll have a look.

@cnrpman cnrpman added the looking-for-review PRs that requires attention label Feb 21, 2023
Copy link
Collaborator

@andelf andelf left a comment

Choose a reason for hiding this comment

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

src/main/frontend/fs/nfs.cljs is not touched.
So some file actions will fail.

@andelf andelf self-assigned this Feb 21, 2023
@andelf
Copy link
Collaborator

andelf commented Feb 21, 2023

@Bad3r To test the filesystem API, use either localhost or https access.

@Bad3r Bad3r added dependencies Pull requests that update a dependency file :type/dev This label is used to indicate that an issue or PR is related to development tasks or changes that labels Feb 21, 2023
@andelf andelf mentioned this pull request Feb 27, 2023
6 tasks
@Bad3r Bad3r removed the looking-for-review PRs that requires attention label Mar 1, 2023
@andelf
Copy link
Collaborator

andelf commented Mar 13, 2023

Many thanks. I'll refactor this in #8792.

UPDATE: won't introduce this for now.

if (files.length && !(files[0] instanceof File)) {
handle = files[0];
} else {
handle = files[0].directoryHandle;
Copy link
Collaborator

Choose a reason for hiding this comment

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

After looking into the implementation, this should be a buggy design decision.
The directoryHandle only binds to the containing directory, not the top directory.

Copy link
Author

Choose a reason for hiding this comment

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

Good this got caught.

@andelf andelf added the :type/hold Hold this PR. won't merge for now label Mar 13, 2023
@andelf andelf removed their assignment May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file :type/dev This label is used to indicate that an issue or PR is related to development tasks or changes that :type/hold Hold this PR. won't merge for now
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants