duktape icon indicating copy to clipboard operation
duktape copied to clipboard

Support for "let"

Open lll000111 opened this issue 6 years ago • 16 comments

"const" already is at least possible to use, even though it's the var semantics. Using let though results in a SyntaxError. I think let could at least be supported like const already is even now, full support notwithstanding.

lll000111 avatar Nov 08 '18 11:11 lll000111

Adding this would be easy, the only question really is whether it will be more confusing to support it but with incorrect scoping (this applies to const already though, as you pointed out).

svaarala avatar Nov 21 '18 15:11 svaarala

Hi,

I've run into a site using these on edbrowse. They'd actually be fine with var semantics here so having that instead of a syntax error.

As you wrote ages ago this is pretty easy, I've monkeyed something that appears to work here if that's of any help (although I'm not sure on how that strings.yaml generates other files?): https://github.com/martinetd/duktape/commit/0701a460ca25c2dc76a96bd3187849ca278d1865

I can open a PR with it after adding some note in the documentation / tests but honestly would prefer the topic of "is a partial feature better than nothing" first; I definitely agree it's confusing, it just so happens that my use case in the wild would work with var instead...

Thanks!

martinetd avatar Jun 25 '19 20:06 martinetd

The diff looks quite good to me at first glance -- feel free to open a PR. I can write some partial feature tests and release documentation as a follow-up if you prefer.

svaarala avatar Jun 25 '19 20:06 svaarala

Hi Guys,

I agree that allowing let keyword without standard behavior may confuse someone. However it would also allow reusing code that uses 'let' and in my books benefits of reusing code outweighs possible confusion.

If I would have a vote, it would go to const-like let support.

heikkitx avatar Jan 24 '20 05:01 heikkitx

I had pretty much totally forgotten about it, started looking at how tests work just now. I probably should have taken svaarala's offer up last year, but if I don't find time to whip some basic tests/doc up by monday I'll just open a PR with what I had and leave the rest up to them... Sorry for the delay!

martinetd avatar Feb 01 '20 00:02 martinetd

@martinetd Let me know how you want to proceed :) No worries about the delay, I'm struggling to find time myself right now also.

svaarala avatar Feb 11 '20 14:02 svaarala

@martinetd Would be awesome if you could create that PR! :)

Small note: It seems to fail for let inside for, e.g.:

let size = 2; // works great
for(let i = 0; i < size; ++i) { // "Syntax Error: parse error"
}           

Squareys avatar Feb 12 '20 13:02 Squareys

@svaarala I tried to take @martinetd's diff and add a test case, but I'm still pretty lost on this: https://github.com/svaarala/duktape/compare/master...Squareys:let-support?expand=1

The ecmatest seem to be failing on current master. Is that expected currently?

(I was able to fix for(let ...), though.)

Squareys avatar Feb 12 '20 19:02 Squareys

@Squareys thanks - please do take over, I'm a bit swamped... I did have a look at how tests run but didn't have the node.js requirements during the little time I could make in a train and that was it since :(

The help is appreciated :)

martinetd avatar Feb 13 '20 20:02 martinetd

If you are OK with Docker, the easiest approach is to use the docker shell:

$ make docker-images
$ make docker-shell-wd

This runs a shell inside a docker image with all dependencies required for development in place.

The work directory will be a copy and any changes are intentionally lost as you exit the Docker shell. (If you want the working copy to be mounted as is, use make docker-shell-wdmount.)

Then inside the shell just:

$ make ecmatest

This should complete with exit code 0. Note that a bunch of known issues are listed, this is expected:

                          test-bi-array-proto-push: fail; 30 diff lines; known issue: array length above 2^32-1 not supported
[...]
SUMMARY: 1109 pass, 32 fail
$ echo $?
0

The summary is a bit misleading, it should say "32 known".

svaarala avatar Feb 15 '20 08:02 svaarala

@Squareys Could you please share solution for:

(I was able to fix for(let ...), though.)

EDIT: OK I found it.

-if (comp_ctx->curr_token.t == DUK_TOK_VAR) {
+if (comp_ctx->curr_token.t == DUK_TOK_VAR || comp_ctx->curr_token.t == DUK_TOK_LET) {

e2iplayer avatar Apr 23 '20 15:04 e2iplayer

@e2iplayer Great, yeah, I have that above link to the diff. Wasn't able to put more time into this, sorry!

Squareys avatar Apr 23 '20 18:04 Squareys

@Squareys Squareys Unfortunately your link is not working most probably because three dots inside.

e2iplayer avatar Apr 23 '20 18:04 e2iplayer

@e2iplayer Uhm, it works fine for me 🤔 Otherwise use https://github.com/Squareys/duktape/tree/let-support

Squareys avatar May 08 '20 14:05 Squareys

@Squareys Please take a look how this link looks like: take a look

As you see it is short cut with dots. I had to find it by myself because of broken link... Anyway thank you very much.

e2iplayer avatar May 08 '20 17:05 e2iplayer

We miss let in the parser. Could we just define let as an alias vor var keyword, so most cases just work fine?

MonkeybreadSoftware avatar Nov 22 '22 06:11 MonkeybreadSoftware