Move filemanager api to new api #2733

ioedeveloper · 2020-04-15T11:29:32Z

Originally #2625

LianaHus · 2020-04-15T13:06:14Z

src/app/files/fileManager.js

@@ -23,7 +23,7 @@ const profile = {
  icon: 'data:image/svg+xml;base64,PD94bWwgdmVyc2lvbj0iMS4wIiBlbmNvZGluZz0idXRmLTgiPz4KPHN2ZyB3aWR0aD0iMTc5MiIgaGVpZ2h0PSIxNzkyIiB2aWV3Qm94PSIwIDAgMTc5MiAxNzkyIiB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciPjxwYXRoIGQ9Ik0xNjk2IDM4NHE0MCAwIDY4IDI4dDI4IDY4djEyMTZxMCA0MC0yOCA2OHQtNjggMjhoLTk2MHEtNDAgMC02OC0yOHQtMjgtNjh2LTI4OGgtNTQ0cS00MCAwLTY4LTI4dC0yOC02OHYtNjcycTAtNDAgMjAtODh0NDgtNzZsNDA4LTQwOHEyOC0yOCA3Ni00OHQ4OC0yMGg0MTZxNDAgMCA2OCAyOHQyOCA2OHYzMjhxNjgtNDAgMTI4LTQwaDQxNnptLTU0NCAyMTNsLTI5OSAyOTloMjk5di0yOTl6bS02NDAtMzg0bC0yOTkgMjk5aDI5OXYtMjk5em0xOTYgNjQ3bDMxNi0zMTZ2LTQxNmgtMzg0djQxNnEwIDQwLTI4IDY4dC02OCAyOGgtNDE2djY0MGg1MTJ2LTI1NnEwLTQwIDIwLTg4dDQ4LTc2em05NTYgODA0di0xMTUyaC0zODR2NDE2cTAgNDAtMjggNjh0LTY4IDI4aC00MTZ2NjQwaDg5NnoiLz48L3N2Zz4=',
  permission: true,
  version: packageJson.version,
-  methods: ['getFolder', 'getCurrentFile', 'getFile', 'setFile', 'switchFile'],
+  methods: ['file', 'exists', 'open', 'writeFile', 'readFile', 'copyFile', 'unlink', 'rename', 'readdir', 'rmdir'],


readdir -> readDir
rmdir -> rmDir

Actually the node api on filesystem use the lowercase here :
https://nodejs.org/dist/latest-v12.x/docs/api/fs.html#fs_fs_readdir_path_options_callback

We are using node api filesystem as a standard for our new api.

why they did half low case half uppercase???

LianaHus · 2020-04-15T13:14:19Z

src/app/files/fileManager.js

+   * Emits error based on error code
+   * @param {object} error error { code, message }
+   */
+  _handleError (error) {


what;s the reason of having this?

So that errors have a uniform format and it is not just thrown anywhere in our codebase.

sounds ok to me, what are the concern about that?

but it adds nothing more to the throw

You're doing three time the job.
For example if the mkdir method throw, the message will be:
"File already exists Cannot create directory ${path}" with the first part beeing handle by the code & the second by the error. But still you have a method _handleExists.

Either you have one method to handle all error with a mapping on the error code :

const errorMsg = { ENOENT: 'No such file or directory', ... } const createError = (err) => new Error(`${errorMsg[err]} ${err.message || ''}`) ... mkdir (path) { if (this.exists(path)) { throw createError({ code: 'EEXIST', message: `Cannot create directory ${path}` }) } }

Or you have a named methods that take a string as argument :

_handleEnoent(msg) { throw new Error('No such file or directory ' + msg); }

Or you can use assert methods by merging check & error :

_assertExist(path, msg) { if (....) { throw new Error('No such file or directory ' + msg); } }

LianaHus · 2020-04-15T13:16:00Z

src/app/files/fileManager.js

+  isFile (path) {
+    const extension = path.split('/').pop()
+
+    return extension && extension.indexOf('.') > -1


you can have browser/myfolder.is.cool folder

i think you should ask provider for this

LianaHus · 2020-04-15T13:20:09Z

src/app/files/fileManager.js

+   * @returns {boolean} true if the path exists
+   */
+  exists (path) {
+    const provider = this.fileProviderOf(path)


this will not work like that, it will return localHost or current provider without checking for path

yann300 · 2020-04-15T13:48:27Z

src/app/files/fileManager.js

+  rmdir (path) {
+    this._handleExists(path, `Cannot remove directory ${path}`)
+    this._handleIsDir(path, `Cannot remove directory ${path}`)
+    // To implement


that will help https://github.com/ethereum/remix-ide/blob/master/src/app/files/fileProvider.js#L144

yann300 · 2020-04-15T13:51:00Z

src/app/files/fileManager.js

+    if (this.exists(path)) {
+      this._handleError({ code: 'EEXIST', message: `Cannot create directory ${path}` })
+    }
+    // To implement


that should do the trick window.remixFileSystem.mkdirSync
or look at https://github.com/ethereum/remix-ide/blob/master/src/app/files/fileProvider.js#L92 for recursively creating folder.

yann300 · 2020-04-15T13:52:19Z

src/app/files/fileManager.js

@@ -23,7 +23,7 @@ const profile = {
  icon: 'data:image/svg+xml;base64,PD94bWwgdmVyc2lvbj0iMS4wIiBlbmNvZGluZz0idXRmLTgiPz4KPHN2ZyB3aWR0aD0iMTc5MiIgaGVpZ2h0PSIxNzkyIiB2aWV3Qm94PSIwIDAgMTc5MiAxNzkyIiB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciPjxwYXRoIGQ9Ik0xNjk2IDM4NHE0MCAwIDY4IDI4dDI4IDY4djEyMTZxMCA0MC0yOCA2OHQtNjggMjhoLTk2MHEtNDAgMC02OC0yOHQtMjgtNjh2LTI4OGgtNTQ0cS00MCAwLTY4LTI4dC0yOC02OHYtNjcycTAtNDAgMjAtODh0NDgtNzZsNDA4LTQwOHEyOC0yOCA3Ni00OHQ4OC0yMGg0MTZxNDAgMCA2OCAyOHQyOCA2OHYzMjhxNjgtNDAgMTI4LTQwaDQxNnptLTU0NCAyMTNsLTI5OSAyOTloMjk5di0yOTl6bS02NDAtMzg0bC0yOTkgMjk5aDI5OXYtMjk5em0xOTYgNjQ3bDMxNi0zMTZ2LTQxNmgtMzg0djQxNnEwIDQwLTI4IDY4dC02OCAyOGgtNDE2djY0MGg1MTJ2LTI1NnEwLTQwIDIwLTg4dDQ4LTc2em05NTYgODA0di0xMTUyaC0zODR2NDE2cTAgNDAtMjggNjh0LTY4IDI4aC00MTZ2NjQwaDg5NnoiLz48L3N2Zz4=',
  permission: true,
  version: packageJson.version,
-  methods: ['getFolder', 'getCurrentFile', 'getFile', 'setFile', 'switchFile'],
+  methods: ['file', 'exists', 'open', 'writeFile', 'readFile', 'copyFile', 'unlink', 'rename', 'readdir', 'rmdir'],


is file a replacement for getCurrentFile

why not current or currentFile? does this come from nodejs as well?

yann300 · 2020-04-15T13:54:31Z

from what i see, there's no issue with parameters ordering right?

ioedeveloper · 2020-04-15T13:59:44Z

@LianaHus left a comment about _handle_error function, What do you think about it? @yann300 @GrandSchtroumpf

ioedeveloper · 2020-04-15T14:02:05Z

from what i see, there's no issue with parameters ordering right?

Yes, i think parameters ordering is okay.

yann300 · 2020-04-15T14:06:36Z

src/app/files/fileManager.js

+   */
+  unlink (path) {
+    this._handleExists(path, `Cannot remove file ${path}`)
+    this._handleIsDir(path, `Cannot remove file ${path}`)


and window.remixFileSystem.unlinkSync(curPath, console.log) should work for this case

yann300 · 2020-04-15T14:09:15Z

btw I've put some comment about using window.remixFileSystem.* . Not saying that we should put window.remixFileSystem.* call in the fileManager. These call should stay in the fileProvider.js .
But that will probably require some changes in fileProvider.js

LianaHus · 2020-05-12T13:25:34Z

src/app/files/file-explorer.js

+            const removeFile = await fileManager.remove(key)
+
+            if (!removeFile) {
+              tooltip(`failed to remove file ${key}.`)


I guess this should be uppercase

src/app/files/fileManager.js

LianaHus · 2020-05-12T13:39:06Z

package.json

@@ -62,7 +62,7 @@
    "remix-solidity": "0.3.30",
    "remix-tabs": "1.0.48",
    "remix-tests": "0.1.33",
-    "remixd": "0.1.8-alpha.10",
+    "remixd": "0.1.8-alpha.16",


I think lock file also needs to be checked in

LianaHus · 2020-05-12T13:44:08Z

src/app/panels/tab-proxy.js

@@ -34,7 +34,7 @@ export class TabProxy {
        return
      }
      this.addTab(file, '', () => {
-        this.fileManager.switchFile(file)
+        this.fileManager.open(file)
        this.event.emit('switchFile', file)


should we also change the name of event to openFile? 'switchFile' makes no sense anymore

Okay, that can be done.

@yann300 Liana suggested we also rename switchFile command for nightwatch to openFile to avoid confusion. What do you think?

Only for nightwatch or for all the codebase? (I mean for the event name too?)
If the changes are well scoped I think that makes sense. If there's a lot of internal rewording changes, I would suggest to split that for another PR.

It's not so much a change. It can go in this PR.

LianaHus · 2020-05-13T16:13:56Z

src/app/files/fileManager.js

+}
+const createError = (err) => {
+  return new Error(`${errorMsg[err.code]} ${err.message || ''}`)
+}

 // File System profile
 // - methods: ['getFolder', 'getCurrentFile', 'getFile', 'setFile', 'switchFile']


could you please remove this comment?

LianaHus

I tried to create a file with vyper and this is not working for me. could you please try this? I don't have any js error either

yann300 · 2020-05-20T07:22:12Z

@LianaHus @ioedeveloper pretty sure it's because the old api has been removed https://github.com/ethereum/remix-ide/pull/2733/files#diff-f28e4232b4dbf7d53c7b5543bdc9ad4fR26

ioedeveloper · 2020-05-21T16:21:24Z

Should be merged after @remixproject/engine release ethereum/remix-plugin#208.

ioedeveloper added the WIP label Apr 15, 2020

ioedeveloper requested review from yann300, GrandSchtroumpf and LianaHus April 15, 2020 11:29

LianaHus reviewed Apr 15, 2020

View reviewed changes

yann300 reviewed Apr 15, 2020

View reviewed changes

ioedeveloper force-pushed the filemanager-api branch 2 times, most recently from de71b61 to 5f5bf40 Compare April 20, 2020 16:36

ioedeveloper force-pushed the filemanager-api branch from 6fde118 to 37f239a Compare May 5, 2020 03:28

LianaHus reviewed May 12, 2020

View reviewed changes

LianaHus requested changes May 12, 2020

View reviewed changes

LianaHus reviewed May 12, 2020

View reviewed changes

ioedeveloper force-pushed the filemanager-api branch from 846e429 to 7855c37 Compare May 13, 2020 09:07

LianaHus reviewed May 13, 2020

View reviewed changes

ioedeveloper added 6 commits May 15, 2020 11:18

Moved getFolder to new API readdir

56a09c9

Added error code and handlers

0e7e1e5

Moved getFile, setFile and switchFile to new API

a545296

Fix linting error

4e3b8d1

Changed logic for file/dir exists to use file provider

df91b92

implemented mkdir and rmdir

9c830a1

ioedeveloper added 17 commits May 15, 2020 11:18

Implemented unlink and changed implementation of error handler

de2a467

Fixed linting error

0488a4c

Fixed broken writeFile function

b5ce812

Removed delay used for debugging

b45004d

Implemented receive response in async/await

12bc629

Changed fileExplorer to async/await to fix remixd error

fcec4fe

Fixed Remixd folder delete bug and changed rmdir, unlink to remove api

4cc3802

Fixed error with isDirectory in file provider

210671f

Fixed linting error

d6fd520

Update remixd version

aa2cd0a

Update remixd dependency in package.json

21e644f

Fixed failing test

30a7e8f

Removed unnecessary comments

ae79ff3

Update terminal test with new filemanager api

b94fe92

Changed switchFile event to openFile event

fc09e7e

Removed comment

10bb3a7

changed nightwatch switchFile command to openFile

2cf7ed5

ioedeveloper force-pushed the filemanager-api branch from 3c39f6b to 2cf7ed5 Compare May 15, 2020 11:19

ensure file is opened before marking it

129bf2e

ioedeveloper removed the WIP label May 19, 2020

LianaHus approved these changes May 19, 2020

View reviewed changes

LianaHus requested changes May 19, 2020

View reviewed changes

ioedeveloper force-pushed the filemanager-api branch from dc55f61 to 129bf2e Compare May 25, 2020 16:50

Implemented old api redirection to new api

4fa4d42

LianaHus approved these changes May 27, 2020

View reviewed changes

yann300 merged commit ecdd1fb into master May 27, 2020

yann300 deleted the filemanager-api branch May 27, 2020 15:25

Move filemanager api to new api #2733

Move filemanager api to new api #2733

Conversation

ioedeveloper commented Apr 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioedeveloper Apr 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioedeveloper Apr 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yann300 commented Apr 15, 2020

ioedeveloper commented Apr 15, 2020 • edited Loading

ioedeveloper commented Apr 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yann300 commented Apr 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LianaHus left a comment

Choose a reason for hiding this comment

yann300 commented May 20, 2020

ioedeveloper commented May 21, 2020 • edited Loading

ioedeveloper commented Apr 15, 2020 •

edited

Loading

ioedeveloper Apr 15, 2020 •

edited

Loading

ioedeveloper Apr 15, 2020 •

edited

Loading

ioedeveloper commented Apr 15, 2020 •

edited

Loading

ioedeveloper commented May 21, 2020 •

edited

Loading