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

cmd/geth, jsre: restore command line editing on windows #1642

Merged
merged 1 commit into from
Aug 14, 2015

Conversation

fjl
Copy link
Contributor

@fjl fjl commented Aug 11, 2015

PR #856 broke command line editing by wrapping stdout with a filter that
interprets ANSI escape sequences to fix colored printing on windows.
Implement the printer in Go instead so it can do its own
platform-dependent coloring.

As a nice side effect, the JS console is now noticeably more responsive
when printing results.

Fixes #1608
Fixes #1612

@robotally
Copy link

Vote Count Reviewers
👍 2 @bas-vk @obscuren
👎 0

Updated: Fri Aug 14 11:13:17 UTC 2015

@fjl
Copy link
Contributor Author

fjl commented Aug 11, 2015

One problem I saw while working on this is that attached consoles get disconnected after 60s
of inactivity. The ipc client doesn't seem to reconnect when geth closes the pipe.

}
}

func (ctx ppctx) printObject(obj *otto.Object, indent string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why you're using this type of "type casting" over obj.Value() and use value.Export?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Export converts both undefined and null to Go nil. It also has other issues, e.g.
new String('foo') exports as map[string]interface{}{"0": "f", "1": "o", "2": "o"}.
Export is recursive and crashes for circular structures. The new printer does, too, but
we can fix it whereas Export is part of otto.

@bas-vk
Copy link
Member

bas-vk commented Aug 12, 2015

👍

@fjl
Copy link
Contributor Author

fjl commented Aug 12, 2015

I'll add circular structure handling so the following doesn't crash the printer:

> a = {'b': 1}
> a.a = a
> a

@fjl fjl force-pushed the fix-js-console-windows branch from e5f20cd to c5c0365 Compare August 12, 2015 08:48
@fjl
Copy link
Contributor Author

fjl commented Aug 12, 2015

@bas-vk PTAL. I've added a depth limit so the above case prints as

> a = {b:1}
{
  b: 1
}
> a.a = a
{
  a: {
    a: {
      a: {
        a: {...},
        b: 1
      },
      b: 1
    },
    b: 1
  },
  b: 1
}

@fjl fjl force-pushed the fix-js-console-windows branch 3 times, most recently from ca856e7 to 1b24061 Compare August 12, 2015 09:19
PR #856 broke command line editing by wrapping stdout with a filter that
interprets ANSI escape sequences to fix colored printing on windows.
Implement the printer in Go instead so it can do its own
platform-dependent coloring.

As a nice side effect, the JS console is now noticeably more responsive
when printing results.

Fixes #1608
Fixes #1612
@fjl fjl force-pushed the fix-js-console-windows branch from 1b24061 to 0ef80bb Compare August 12, 2015 10:06
func (ctx ppctx) doOwnProperties(v otto.Value, f func(string)) {
Object, _ := ctx.vm.Object("Object")
rv, _ := Object.Call("getOwnPropertyNames", v)
gv, _ := rv.Export()

Choose a reason for hiding this comment

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

What does rv and gv mean? Is the second returned value here errors? If so why are they not checked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rv is short for return value, gv for go value. None of the errors are checked because
Object.getOwnPropertyNames exists as a method and should not error for JS objects.
Try breaking it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The easiest way to break this is to evaluate Object.getOwnPropertyNames = null at the console.
Printing any object after that crashes the console. I don't think it is worth fixing though. The best
thing we could do in such a situation would be to print "can't print because you broke Object".

@fjl fjl added this to the 1.0.2 milestone Aug 12, 2015
@bas-vk
Copy link
Member

bas-vk commented Aug 13, 2015

Maybe we should print such a message and inform the user that its probably best to restart geth or the console in case of an attach. Now it crashes which might cause issues.

The depth limit change is ok.

@obscuren
Copy link
Contributor

👍

obscuren added a commit that referenced this pull request Aug 14, 2015
cmd/geth, jsre: restore command line editing on windows
@obscuren obscuren merged commit 3a66c4e into develop Aug 14, 2015
@obscuren obscuren removed the review label Aug 14, 2015
@bas-vk bas-vk mentioned this pull request Aug 14, 2015
@fjl fjl mentioned this pull request Aug 14, 2015
@obscuren obscuren deleted the fix-js-console-windows branch August 19, 2015 22:33
maoueh pushed a commit to streamingfast/go-ethereum that referenced this pull request Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants