-
Notifications
You must be signed in to change notification settings - Fork 703
fix(1632): erase const enums after inlining #1639
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
Conversation
798d12b
to
c5a62a9
Compare
441cca9
to
ded3fa0
Compare
...ference/submodule/compiler/blockScopedEnumVariablesUseBeforeDef_verbatimModuleSyntax.js.diff
Outdated
Show resolved
Hide resolved
testdata/baselines/reference/submodule/compiler/constEnumErrors.js.diff
Outdated
Show resolved
Hide resolved
testdata/baselines/reference/submodule/conformance/constEnum4.js.diff
Outdated
Show resolved
Hide resolved
testdata/baselines/reference/submodule/conformance/constEnum4.js
Outdated
Show resolved
Hide resolved
...baselines/reference/submodule/compiler/elidedEmbeddedStatementsReplacedWithSemicolon.js.diff
Outdated
Show resolved
Hide resolved
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.
So, this seems probably right but will defer to @weswigham 😄
@jakebailey ok, thanks |
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.
I was unsure if we wanted this behavior back and didn't just wanna make const enum
s always preserved, since that's what we were doing before we re-added inlining, but I suppose since vscode reported it, they'd like the runtime source elision behavior back, too. Tbh, preserveConstEnums
should at least be the default nowadays, but that's a change for another PR.
We also need to elide namespace
declarations containing exclusively const enum
s and/or types (preserveConstEnums
dependent) for the same reasons you'd want the const enum
s elided. See the missing shouldEmitModuleDeclaration
call from the original ts.ts
source. ast.GetModuleInstanceState
is already ported, so it shouldn't be too bad.
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.
Pretty good - some last comments on comment preservation, since we're working with NotEmittedStatement
s which explicitly exist to copy those.
Fixes #1632