Skip to content

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 <[email protected]>
['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)

@bjohansebas bjohansebas added the stalled Issues and PRs that are stalled. label Mar 28, 2025
Copy link
Contributor

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

@github-actions github-actions bot removed the stalled Issues and PRs that are stalled. label Apr 3, 2025
@BridgeAR
Copy link
Member

The mentioned issue seems to be resolved by another PR.
I believe this could therefore be closed? It would otherwise need a rebase. If I am wrong, please open a new PR, since it's more likely to be picked up that way again!

@BridgeAR BridgeAR closed this Sep 11, 2025
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
5 participants