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

doc: update example of using await in REPL #45939

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

deokjinkim
Copy link
Contributor

@deokjinkim deokjinkim commented Dec 21, 2022

Lexical scope of const is only invalidated when
using top-level await in REPL.

Fixes: #45918

Invalidating the lexical scoping of the `const` and
`let` keywords is fixed.

Fixes: nodejs#45918
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. repl Issues and PRs related to the REPL subsystem. labels Dec 21, 2022
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Can we add a test for that?

@deokjinkim
Copy link
Contributor Author

Can we add a test for that?

Okay. I'll try to write test case for that.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

The scope is still changed, if I am not mistaken:

> const m = await Promise.resolve(123)
undefined
> m
123
> m = 'Lexial scope changed!'
'Lexial scope changed!'
> m
'Lexial scope changed!'
> 

vs.

> const m = 123
undefined
> m
123
> m = 'Lexial scope changed!'
Uncaught TypeError: Assignment to constant variable.
> 

@deokjinkim
Copy link
Contributor Author

@jasnell As I checked, you are author of this change(#38449). Do you have any idea for this issue?

Lexical scope of `const` is only invalidated when
using top-level `await` in REPL.

Fixes: nodejs#45918
@deokjinkim deokjinkim force-pushed the 221222_remove_limitation_await_repl branch from 31508ee to 2de633c Compare December 23, 2022 09:55
@deokjinkim deokjinkim changed the title doc: remove limitation of using await in REPL doc: update example of using await in REPL Dec 23, 2022
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

The change is LGTM. I can't reproduce any other behavior anymore.
Ideally we could add a test case that validates the outcome accordingly.

@BridgeAR BridgeAR dismissed their stale review January 2, 2023 17:22

See comment above

@deokjinkim
Copy link
Contributor Author

The change is LGTM. I can't reproduce any other behavior anymore. Ideally we could add a test case that validates the outcome accordingly.

@BridgeAR @aduh95 Added test case according to change of document in REPL. Could you please review again?

@deokjinkim deokjinkim requested review from BridgeAR and aduh95 and removed request for BridgeAR and aduh95 January 4, 2023 23:16
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
['const k = await Promise.resolve(123)'],
['k', '123'],
['k = await Promise.resolve(234)', '234'],
['k', '234'],
Copy link
Contributor

Choose a reason for hiding this comment

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

And the drive the point home:

Suggested change
['k', '234'],
['k', '234'],
['const k = await Promise.resolve(345)'],
['k', '345'],

Copy link
Contributor Author

Choose a reason for hiding this comment

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

line 180 causes error like 'Uncaught SyntaxError: Identifier 'k' has already been declared'.
When I tested,

// This is allowed
const k = await Promise.resolve(123)
k = await Promise.resolve(234)

// This is not allowed
const k = await Promise.resolve(123)
const k = await Promise.resolve(234)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lexical scoping of the const is not invalidated when use await in REPL
4 participants