Add git service in remixd & use it from the terminal #638

yann300 · 2020-11-26T12:48:40Z

GrandSchtroumpf · 2020-11-30T09:57:48Z

apps/remix-ide/src/app/files/git-handle.js

+    super(profile)
+  }
+
+  deactivate () {


Remove deactivate & activate if you only forward to extended class

GrandSchtroumpf · 2020-11-30T10:01:51Z

apps/remix-ide/src/app/files/git-handle.js

+  displayName: 'Git',
+  url: 'ws://127.0.0.1:65521',
+  methods: ['command'],
+  events: [],


events is deprecated. You can remove it

GrandSchtroumpf · 2020-11-30T10:05:22Z

apps/remix-ide/src/app/panels/terminal.js


    function wrapScript (script) {
-      if (script.startsWith('remix.')) return script
+      if (script.trim().startsWith('remix.') || script.trim().startsWith('git')) return script


Create a variable to make it easier to read:

const isRemix = script.trim().startsWith('remix.') const isGit = script.trim().startsWith('git') if (isRemix || isGit) return script;

or

const isKnownScript = ['remix.', 'git'].some(prefix => script.trim().startWith(prefix)) if (isKnownScript) return script

GrandSchtroumpf · 2020-11-30T10:06:36Z

apps/remix-ide/src/app/panels/terminal.js

    }
    try {
-      await this.call('scriptRunner', 'execute', script)
+      if (script.trim().indexOf('git') === 0) {


Keep the same logic everywhere. Use startWith instead of indexOf() (except if you need the index for later, which is not the case here).

GrandSchtroumpf · 2020-11-30T10:10:51Z

apps/remix-ide/src/app/files/git-handle.js

+  name: 'git',
+  displayName: 'Git',
+  url: 'ws://127.0.0.1:65521',
+  methods: ['command'],


Why command while the scriptRunner method is execute ? I think it makes it difficult for developers to follow up with the naming.
I would recommend to define a kind "terminal" or whatever with the right interface.

GrandSchtroumpf · 2020-11-30T10:27:26Z

libs/remixd/src/services/gitClient.ts

+  }
+
+  command (cmd: string) {
+    return new Promise((resolve, reject) => {


I would recommend to put the Promise closer to its used :

assertCommand(cmd, gitRegex) const options = { cwd: this.currentSharedFolder, shell: true } const child = spawn(cmd, options) let result = '' let error = '' return new Promise((resolve, reject) => { child.stdout.on('data', (data) => result += data.toString()) child.stderr.on('data', (err) => error += data.toString()) child.on('close', (exitCode) => exitCode !== 0 ? reject(error) : resolve(result)) })

You don't need to try / catch here everything that throw will be handled by the caller

GrandSchtroumpf · 2020-11-30T10:27:41Z

libs/remixd/src/services/gitClient.ts

+ * @param cmd
+ * @param regex
+ */
+function validateCommand (cmd, regex) {


rename assertCommand and remove the regex parameter, except if you see a reason where it could be different than the one at the top of the file.

GrandSchtroumpf · 2020-11-30T10:28:52Z

libs/remixd/src/services/gitClient.ts

+import * as WS from 'ws' // eslint-disable-line
+import { PluginClient } from '@remixproject/plugin'
+const { spawn } = require('child_process')
+const gitRegex = '^git\\s[^&|;]*$'


Put the regex in the assertion function directly.

GrandSchtroumpf · 2020-11-30T10:30:27Z

libs/remixd/src/websocket.ts

 import * as WS from 'ws'
 import * as http from 'http'
-import { WebsocketOpt, SharedFolderClient } from './types' // eslint-disable-line
+import { WebsocketOpt, ServiceClient } from './types' // eslint-disable-line


any reason why you need to disable line ? I'm not sure you need to do that anymore.

GrandSchtroumpf · 2020-11-30T10:31:42Z

libs/remixd/src/websocket.ts

-    this.server.listen(this.port, loopback, function () {
-      console.log((new Date()) + ' remixd is listening on ' + loopback + ':65520')
+    this.server.listen(this.port, loopback, () => {
+      console.log((new Date()) + ' remixd is listening on ' + loopback + ':' + this.port + '')


I would looks a little bit cleaner with a string template I think :

console.log(`${new Date()} remixd is listening on ${loopback}:${this.port}`)

ioedeveloper · 2020-11-30T10:36:46Z

E2E looks good.

GrandSchtroumpf · 2020-12-07T11:37:44Z

apps/remix-ide/src/app/files/remixd-handle.js

          }
        }, 3000)
        this.locahostProvider.init(_ => this.fileSystemExplorer.ensureRoot())
+        this.appManager.ensureActivated('git')


This has been removed in #651. Why not running this.call('manager', 'activatePlugin', 'git') ?
Or even let the engine lazily activate the git plugin on first call

GrandSchtroumpf · 2020-12-07T11:38:52Z

apps/remix-ide/src/app/files/remixd-handle.js

  deactivate () {
    this.fileSystemExplorer.hide()
    if (super.socket) super.deactivate()
+    this.appManager.ensureDeactivated('git')


Any reason why not using this.call('manager', 'deactivatePlugin', 'git') ?
Use call through the engine when possible instead of direct call to a plugin.

GrandSchtroumpf · 2020-12-07T11:40:27Z

apps/remix-ide/src/app.js

    test,
-    filePanel.remixdHandle
+    filePanel.remixdHandle,
+    filePanel.gitHandle


as a Plugin I would recommend putting Git at the root of the project and access its methods only through remix-plugin API

will plan this changes later on. This will anyway go in during the react refactoring.

GrandSchtroumpf · 2020-12-07T11:41:01Z

apps/remix-ide/src/app/panels/file-panel.js

    var fileSystemExplorer = createProvider('localhost')

    self.remixdHandle = new RemixdHandle(fileSystemExplorer, self._deps.fileProviders.localhost, appManager)
+    self.gitHandle = new GitHandle()


I would recommend to put GitHandle at the root of the project and not in the FilePanel property

GrandSchtroumpf · 2020-12-07T11:43:34Z

apps/remix-ide/src/app/panels/terminal.js

+        const result = await this.call('git', 'command', script)
+        self.commands.html(yo`<pre>${result}</pre>`)
+      } else {
+        await this.call('scriptRunner', 'execute', script)


You can increase the timeout of the scriptRunner plugin and return the value. It doesn't prevent you to listen on log events if you need

GrandSchtroumpf · 2020-12-07T11:56:24Z

libs/remixd/src/services/gitClient.ts

+    })
+    child.on('close', () => {
+      if (error !== '') this.emit('error', error)
+      else this.emit('log', result)


I think returning a value was a better way to handle it.

yann300 requested a review from ioedeveloper November 26, 2020 12:48

yann300 force-pushed the addgitService branch 5 times, most recently from 73ca6be to 002c405 Compare November 26, 2020 14:52

yann300 added enhancement New feature or request remix-ide remixD WIP labels Nov 26, 2020

yann300 force-pushed the addgitService branch 5 times, most recently from 7b11e96 to dd4e761 Compare November 27, 2020 11:14

yann300 removed the WIP label Nov 27, 2020

yann300 force-pushed the addgitService branch 2 times, most recently from e3ebc65 to 441b4b0 Compare November 27, 2020 12:18

yann300 requested review from Aniket-Engg and LianaHus November 27, 2020 12:39

GrandSchtroumpf suggested changes Nov 30, 2020

View reviewed changes

ioedeveloper approved these changes Nov 30, 2020

View reviewed changes

yann300 force-pushed the addgitService branch from c10759d to 7c5c860 Compare December 1, 2020 15:30

yann300 requested a review from GrandSchtroumpf December 2, 2020 08:33

GrandSchtroumpf reviewed Dec 7, 2020

View reviewed changes

GrandSchtroumpf approved these changes Dec 7, 2020

View reviewed changes

yann300 added 3 commits December 7, 2020 15:08

add git service in remixd & use it from the terminal

c767721

don't use extend Auto completion (that's spamming the autocomplete)

4ead915

standard

80a9e76

yann300 added 13 commits December 7, 2020 15:08

fix e2e tests

658d4c3

removed uneeded implementation

7d02418

update method name

80c270c

better string check

c44eb3a

standard

d3e59ec

refactor

850d85f

refactor

baf86d9

git has execute api

dcef83a

listen on event

23139d4

standard

40484a3

change emitting event

5c49808

standard

6750955

standard

eb64ab6

yann300 force-pushed the addgitService branch from 0c314a5 to eb64ab6 Compare December 7, 2020 14:09

yann300 added 2 commits December 7, 2020 15:31

remove ensureDeactivated

5c58b48

git return value insead of logging

a789fa7

yann300 force-pushed the addgitService branch 2 times, most recently from 421672e to 189981d Compare December 7, 2020 15:03

standard

7d1fe50

yann300 force-pushed the addgitService branch from 189981d to 7d1fe50 Compare December 7, 2020 15:24

yann300 merged commit 25b83af into master Dec 7, 2020

Add git service in remixd & use it from the terminal #638

Add git service in remixd & use it from the terminal #638

Uh oh!

Conversation

yann300 commented Nov 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ioedeveloper commented Nov 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yann300 commented Nov 26, 2020 •

edited

Loading