-
Notifications
You must be signed in to change notification settings - Fork 3k
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(forEach): fix a temporal dead zone issue in forEach. #2474
Conversation
src/Observable.ts
Outdated
const subscription = this.subscribe((value) => { | ||
// Must be explicitly assigned to avoid RefernceError when accessing | ||
// subscription below in the closure due to Temporal Dead Zone. | ||
let subscription: Subscription = undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let subscription: Subscription;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
According to the ES6 spec and the implementation in V8, accessing a lexically scoped construct like const or let its declaration has finished is a ReferenceError. Unlike var, it is not implicitly undefined in the entire function scope. Because the closure handed to subscribe is immediately invoked and accesses `subscription` before it is assigned, this causes a ReferenceError in compliant engines. The error is only triggered when running in plain ES6, i.e. without transpilation.
Curiousity question: how previous code can be executed without transpilation to hit those errorneus cases? |
@kwonoj if you run the code in an ES6 VM, it'll fail. But you can repro this much more easily locally: > function call(c) { return c(); }
undefined
> function undef() { const foo = call(() => foo ? 1 : 2); }
undefined
> undef()
ReferenceError: foo is not defined
at call (repl:1:55)
at call (repl:1:38)
at undef (repl:1:44)
at repl:1:1
at ContextifyScript.Script.runInThisContext (vm.js:23:33)
at REPLServer.defaultEval (repl.js:340:29)
at bound (domain.js:280:14)
at REPLServer.runBound [as eval] (domain.js:293:12)
at REPLServer.onLine (repl.js:537:10)
at emitOne (events.js:101:20)
> function undef() { let foo; foo = call(() => foo ? 1 : 2); }
undefined
> undef()
undefined |
Ah, question was since code is TS anyway, it should be transpiled via compiler so curious how did original could be executed. Just curiousity question though. |
If you transpile TypeScript with target language ES6, you get ES6 :-)
|
Oh thought compiler does something like hoisting automatically. Thx for clarification. |
@kwonoj can you merge? I don't have write access. |
No, I intentionally left this to get second eye and release planning. /cc @benlesh |
thanks @mprobst! |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
According to the ES6 spec and the implementation in V8, accessing a lexically scoped construct like const or let before it is assigned is a ReferenceError. Unlike var, it is not implicitly undefined. Because the closure handed to subscribe is immediately invoked and accesses
subscription
before it is assigned, this causes a ReferenceError in compliant engines.The error is only triggered when running in plain ES6, i.e. without transpilation.