BREAKING CHANGE: Refactor fs #8792

andelf · 2023-03-09T09:12:06Z

Warning
This PR rewrites some of the underlying fs implementation and path handling.

Requires at least 3 or more devs' approval.

andelf · 2023-03-14T10:13:16Z

Now the basic refactoring is done and ready for early review.

How to Test

This PR is a BREAKING CHANGE, Clear Cache is required

Platform Checklist

Electron
Electron on Windows
Android
iOS
Demo graph (on-boarding) on all platforms
Browser (Chrome)

Operations

Open Graph
Refresh Graph
Re-index Graph
Basic block editing
File creating & deleting & renaming
Asset adding & deleting
Config editing
Global Config editing
Special page names, on macOS/Linux, Windows, Android, iOS
Special graph names, on macOS/Linux, Windows, Android, iOS
Special graph paths, on macOS/Linux, Windows, Android, iOS
Work with Logseq Sync

deps/common/src/logseq/common/path.cljs

deps/common/src/logseq/common/path_test.cljs

src/electron/electron/utils.cljs

andelf · 2023-03-16T14:25:40Z

UPDATE: There won't be more functional and refactoring commits.
I'll fix the CI tests.

cnrpman · 2023-03-16T15:58:41Z

About the E2E CI failure:
During onboarding, when attempting to bypass the file selector using the MOCKED_OPEN_DIR_PATH var in E2E, the behavior is not expected.
Can be test via:

Clear cache
Set the window.MOCKED_OPEN_DIR_PATH value to "/Users/jun/git/lambda", by executing following in console:

window.__MOCKED_OPEN_DIR_PATH__ = "/Users/jun/git/lambda"

Mouse click "choose a folder"

The behavior on the master branch is normal, but it is wrong in the pull request.

Related code:

logseq/src/main/frontend/util.cljc

Lines 140 to 144 in de2c1f7

    
           #?(:cljs 
        
              (defn mocked-open-dir-path 
        
                "Mocked open DIR path for by-passing open dir in electron during testing. Nil if not given" 
        
                [] 
        
                (when (electron?) (. js/window -__MOCKED_OPEN_DIR_PATH__))))

logseq-cldwalker

@andelf Thanks for taking on this necessary and tricky refactor. I haven't finished reviewing this large PR but figured I'd share my feedback so far

logseq-cldwalker · 2023-03-20T15:09:40Z

src/main/frontend/components/block.cljs

@@ -155,6 +156,7 @@

 (defn- get-file-absolute-path
  [config path]
+  (js/console.error "TODO: buggy path fn")


Can this be removed?

will remove this in the next PR.

logseq-cldwalker · 2023-03-20T15:47:34Z

src/main/frontend/config.cljs

-         (->> (take-last 2 (string/split repo-url #"/"))
-              (string/join "_")))))
+    (do
+      (js/console.error "BUG: This should be unreachable! get-repo-dir" repo-url)


If this shouldn't happen, should we also notify sentry so we know this is happening?

This is a notification to devs, not a fatal error. To standardize the use of get-repo-dir.

src/main/frontend/config.cljs

src/main/frontend/core.cljs

src/main/frontend/fs/memory_fs.cljs

src/main/frontend/fs.cljs

logseq-cldwalker · 2023-03-20T21:10:02Z

deps/common/src/logseq/common/path.cljs

@@ -0,0 +1,302 @@
+(ns logseq.common.path


I think it's great we're trying to improve file and path handling. However, this is the 3rd major approach to file and path handling after frontend.util/node-path.* and the npm path library. Without some documentation it's unclear when to use which approach. Could you write some docs on when to use this ns? Some questions that would be helpful to answer in docs:

Is there a plan to deprecate the other approaches and if so, what is it?

When to use filename vs util/node-path.basename?

When to use file-ext vs gp-util/path->file-ext?

When to use path-join vs util/node-path.join or node-path/join? What type of paths should we not use join for? I've seen relative paths like logseq/config.edn aren't using it so maybe it's only for full paths?

What platforms do these fns support? For example, is relative-path mostly for mobile since it's replacing capacitor-fs/replacing normalize-file-protocol-path?

logseq-cldwalker · 2023-03-20T21:18:03Z

deps/common/src/logseq/common/path_test.cljs

@@ -0,0 +1,27 @@
+(ns logseq.common.path-test


Could you add tests for more of the main fns? Perhaps some of the deleted tests of capacitor-fs-test can be reused. I'd be hesitant to use this library going forward if there's little shared understanding of what the expected behavior is. I can help with desktop tests if you'd like

src/main/frontend/handler/global_config.cljs

logseq-cldwalker · 2023-03-21T21:28:59Z

@andelf Added a commit to setup tests and fix minor stuff. Just trying to help on workflow so feel free to change it if you have a preferred setup. Should be able to do QA and finish review tomorrow

logseq-cldwalker

@andelf QAed desktop and browser and it works great except for config issues. I fixed global config issues I was seeing. I see that config editing fails to open in the browser and confirmed this was occurring before my commits. Back to you for that bug and some comments. Can help with desktop tests tomorrow if you'd like

src/main/frontend/db/model.cljs

logseq-cldwalker · 2023-03-22T18:10:40Z

src/main/frontend/handler/file_sync.cljs

@@ -1,6 +1,6 @@
 (ns frontend.handler.file-sync
  "Provides util handler fns for file sync"
-  (:require ["path" :as path]
+  (:require ["path" :as node-path]


Good idea to rename to avoid having conflict aliases. Happy to finish doing these renames in a follow up PR

- basic clj-kondo config and cleanup unused binding - Add some unit tests - Add a basic readme

andelf changed the title ~~Refactor fs~~ BREAKING CHANGE: Refactor fs Mar 11, 2023

andelf marked this pull request as ready for review March 11, 2023 09:48

andelf changed the title ~~BREAKING CHANGE: Refactor fs~~ WIP: BREAKING CHANGE: Refactor fs Mar 11, 2023

andelf mentioned this pull request Mar 13, 2023

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

Open

andelf requested review from tiensonqin, logseq-cldwalker, cnrpman, RCmerci and situ2001 March 14, 2023 10:07

andelf changed the title ~~WIP: BREAKING CHANGE: Refactor fs~~ BREAKING CHANGE: Refactor fs Mar 14, 2023

andelf added this to the 0.8.19 milestone Mar 14, 2023

RCmerci reviewed Mar 16, 2023

View reviewed changes

deps/common/src/logseq/common/path.cljs Outdated Show resolved Hide resolved

deps/common/src/logseq/common/path_test.cljs Outdated Show resolved Hide resolved

src/electron/electron/utils.cljs Show resolved Hide resolved

andelf force-pushed the refactor/fs2 branch 2 times, most recently from 89c9514 to be639e1 Compare March 17, 2023 05:25

This was referenced Mar 17, 2023

Asset upload not working with web app #8840

Closed

fix(pdf): incorrect filename encoding #8843

Open

tiensonqin force-pushed the refactor/fs2 branch from dffe301 to a62ca64 Compare March 20, 2023 10:07

logseq-cldwalker reviewed Mar 20, 2023

View reviewed changes

andelf force-pushed the refactor/fs2 branch from ccdb4f6 to 1980527 Compare March 21, 2023 14:39

logseq-cldwalker requested changes Mar 23, 2023

View reviewed changes

tiensonqin and others added 12 commits March 28, 2023 08:52

fix: can't pull remote graphs on iOS

34da87f

fix(fs): handle empty dir for get-files

99095ee

fix(fs): disable perm check for nfs read-file

fc13238

fix: can't save config.edn on web

08d3b2d

fix: config.edn path on settings

51eed80

refactor: path/absolute?

f7b3f66

refactor(fs): simplify path/absolute?

47fd784

refactor(migration): add file/path migration

b34b6aa

fix(db): wrong schema/version query

5a811da

chore: cleanup fixme

92a23d5

chore: address comments

a9e7a5d

Minor clean up and help with feedback

fa9001f

- basic clj-kondo config and cleanup unused binding - Add some unit tests - Add a basic readme

andelf force-pushed the refactor/fs2 branch from 9d44410 to fa9001f Compare March 28, 2023 00:54

andelf merged commit 06fa658 into master Mar 28, 2023
17 checks passed

Bad3r mentioned this pull request Mar 28, 2023

Open in directory fails after fs refactor #8922

Closed

tiensonqin deleted the refactor/fs2 branch March 28, 2023 12:11

logseq-cldwalker mentioned this pull request Mar 29, 2023

Chore: Consistent "path" aliases #8943

Merged

cnrpman mentioned this pull request May 24, 2023

I can't open pdf file in logseq 0.8.9 with "\" file path #7123

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BREAKING CHANGE: Refactor fs #8792

BREAKING CHANGE: Refactor fs #8792

andelf commented Mar 9, 2023 •

edited

andelf commented Mar 14, 2023 •

edited

andelf commented Mar 16, 2023

cnrpman commented Mar 16, 2023 •

edited

logseq-cldwalker left a comment

logseq-cldwalker Mar 20, 2023

andelf Mar 28, 2023

logseq-cldwalker Mar 20, 2023

andelf Mar 28, 2023

logseq-cldwalker Mar 20, 2023 •

edited

logseq-cldwalker Mar 20, 2023

logseq-cldwalker commented Mar 21, 2023

logseq-cldwalker left a comment

logseq-cldwalker Mar 22, 2023

BREAKING CHANGE: Refactor fs #8792

BREAKING CHANGE: Refactor fs #8792

Conversation

andelf commented Mar 9, 2023 • edited

andelf commented Mar 14, 2023 • edited

How to Test

Platform Checklist

Operations

andelf commented Mar 16, 2023

cnrpman commented Mar 16, 2023 • edited

logseq-cldwalker left a comment

Choose a reason for hiding this comment

logseq-cldwalker Mar 20, 2023

Choose a reason for hiding this comment

andelf Mar 28, 2023

Choose a reason for hiding this comment

logseq-cldwalker Mar 20, 2023

Choose a reason for hiding this comment

andelf Mar 28, 2023

Choose a reason for hiding this comment

logseq-cldwalker Mar 20, 2023 • edited

Choose a reason for hiding this comment

logseq-cldwalker Mar 20, 2023

Choose a reason for hiding this comment

logseq-cldwalker commented Mar 21, 2023

logseq-cldwalker left a comment

Choose a reason for hiding this comment

logseq-cldwalker Mar 22, 2023

Choose a reason for hiding this comment

andelf commented Mar 9, 2023 •

edited

andelf commented Mar 14, 2023 •

edited

cnrpman commented Mar 16, 2023 •

edited

logseq-cldwalker Mar 20, 2023 •

edited