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

fix(cli/repl): interpret object literals as expressions #7591

Conversation

caspervonb
Copy link
Contributor

@caspervonb caspervonb commented Sep 20, 2020

This wraps object literals in parenthesis so that they're interpreted as expressions rather than block statements which matches the behaviour of Node and web-browsers.

Fixes #1421

@caspervonb caspervonb force-pushed the fix-repl-interpret-object-literals-as-expressions branch from 75f0e74 to 1fc75d3 Compare September 20, 2020 05:48
@caspervonb
Copy link
Contributor Author

Old behavior has some interesting edge cases

Master:

Screenshot 2020-09-20 at 1 59 22 PM

This pull request:

Screenshot 2020-09-20 at 2 01 41 PM

Copy link
Collaborator

@nayeemrmn nayeemrmn left a comment

Choose a reason for hiding this comment

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

Fixes #1421. I suppose this is better even if block statements can't be used.

cli/rt/40_repl.js Outdated Show resolved Hide resolved
Co-authored-by: Nayeem Rahman <nayeemrmn99@gmail.com>
@kitsonk
Copy link
Contributor

kitsonk commented Sep 20, 2020

The existing behaviour is consistent with Node.js. Is there compelling reason to vary from that?

@lucacasonato
Copy link
Member

The existing behaviour is consistent with Node.js. Is there compelling reason to vary from that?

It is not.
image

@kitsonk
Copy link
Contributor

kitsonk commented Sep 20, 2020

Ah, but { "" } does not throw a syntax error though.

@kitsonk
Copy link
Contributor

kitsonk commented Sep 20, 2020

Also, { "" } in the dev console in Chrome doesn't throw either. It returns "".

Maybe a RegEx that is a bit more specific in looking for things that look like an object literal but ignores things that couldn't be an object literal. The following:

/^\s*{\s*(?:\S+|["'][^"']+["']):.+}\s*$/

Matches:

{ "foo": bar }
{ 'foo bar': bar }
{ [foo]: bar }

But ignores:

{ "" }

It isn't perfect, but it is really hard to make it perfect because of the lack of back references in JavaScript.

@caspervonb
Copy link
Contributor Author

The existing behaviour is consistent with Node.js. Is there compelling reason to vary from that?

Node does the same, but with a different pattern match.

Also, { "" } in the dev console in Chrome doesn't throw either. It returns "".

Throws in Safari, not sure about Firefox (not installed at the moment).

/^\s*{\s*(?:\S+|["'][^"']+["']):.+}\s*$/

Yeah that regex is going to be pretty hard to get right, it should allow for non-quoted keys, but non-quoted keys look like labels, et cetera.

I think it might be better to have a rollback mechanism, e.g if our preprocessed code turned out to be a syntax error, do it again without the preprocessing.

@caspervonb
Copy link
Contributor Author

Slightly more forgiving approach that leaves the parsing up to the compiler; if it looks like it might be an object literal we try it as an expression first, if thats a compile error we rollback and retry with the raw original code.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

Cool - nice hack.

@ry ry merged commit 9caeff3 into denoland:master Sep 21, 2020
@caspervonb caspervonb deleted the fix-repl-interpret-object-literals-as-expressions branch May 24, 2021 13:48
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.

REPL emits surprising output on > {a: 1}
5 participants