Preserve const enums should keep import refs #28498

weswigham · 2018-11-13T02:46:43Z

For exported const enums exported via export default.

markExportAsReferenced fully resolves aliases and so doesn't mark the import when the symbol is brought in from another file like in the given example. export {...} doesn't worry about this because for an export {...}, it is an error to export something not declared within the current file (it exports bindings, not expressions). export default <expr>, however, allows exporting arbitrary expressions - so the symbol associated with the expression likely won't be the same as the alias associated with the export assignment itself.

Meanwhile, markAliasReferenced also didn't consider aliases of const enum-y namespaces as references for preserving imports while in preserveConstEnums mode.

sheetalkamat · 2018-11-13T16:36:09Z

src/compiler/checker.ts

@@ -16014,7 +16014,7 @@ namespace ts {
        }

        function markAliasReferenced(symbol: Symbol, location: Node) {
-            if (isNonLocalAlias(symbol, /*excludes*/ SymbolFlags.Value) && !isInTypeQuery(location) && !isConstEnumOrConstEnumOnlyModule(resolveAlias(symbol))) {
+            if (isNonLocalAlias(symbol, /*excludes*/ SymbolFlags.Value) && !isInTypeQuery(location) && (compilerOptions.preserveConstEnums || !isConstEnumOrConstEnumOnlyModule(resolveAlias(symbol)))) {


Don't you need similar change in markExportAsReferenced ? Also I think we should remove the isConstEnumOrConstEnumOnlyModule check or update it before calling markAliasSymbolAsReferenced in checkJsxOpeningLikeElementOrOpeningFragment, checkPropertyAccessExpressionOrQualifiedName, markEntityNameOrEntityExpressionAsReference,

Don't you need similar change in markExportAsReferenced ?

Actually, I don't need to call markExportAsReferenced anymore. The markAliasReferenced is more correct for an export assignment (as it mimics what checkIdentifier would do). markExportAsReferenced looks at the wrong symbol for an export assignment (it looks at the default export symbol rather than the symbol of the identifier in the expression), so it hasn't been doing any meaningful work anyway. I'll remove its handling of them. (And you know what? The checking functions built into markAliasSymbolAsReferenced are strange - I'll try to move those so they're local to what they check, too)

Also I think we should remove the isConstEnumOrConstEnumOnlyModule check or update it before calling markAliasSymbolAsReferenced in checkJsxOpeningLikeElementOrOpeningFragment,

I don't know why we check const-ness here, actually. I'm pretty sure we just need to always consider it referenced, const or no (since we need to preserve the import because we'll be referencing it for the factory function in the emit).

checkPropertyAccessExpressionOrQualifiedName

I don't think so - the check there is just checking prop to see if it's even possible for parentSymbol to be const - a fast path. It calls into markAliasReferenced which does the full check.

markEntityNameOrEntityExpressionAsReference

No need - this is only used for decorator metadata, which I think we'd rather have been the same for preserve const enums/non preserved (and const enums are just going to be mapped to Number and their namespaces Object anyway - unlike other types which may have necessitated keeping imports for classes of some sort; so we don't want their imports kept in preserve mode, since we won't use them!).

MLoughry · 2019-05-13T14:48:41Z

@weswigham What's the likelihood this makes it into TS 3.5? This bug is preventing us from upgrading to more recent versions of webpack, which must have added validation that errors on conditions (such as this) where a file exports a symbol that's not defined.

weswigham · 2019-05-13T18:17:12Z

preventing us from upgrading to more recent versions of webpack

??? How so? afaik webpack doesn't use any const enums... (and if it somehow did, they shouldn't be in their public API, as putting const enums in a public API is bad for compatibility)?

MLoughry · 2019-05-13T18:39:58Z

The emitted JS (due to this bug) has an export for a symbol that is not defined in the file (because the import was removed by TS). In our specific case, we may no longer need “preserveConstEnums”; if so, we can work around that way. However, it’s still a bug that blocks newer webpack versions for people who still need that setting. - Michael

…

________________________________ From: Wesley Wigham <[email protected]> Sent: Monday, May 13, 2019 11:18:48 AM To: microsoft/TypeScript Cc: Michael Loughry; Comment Subject: Re: [microsoft/TypeScript] Preserve const enums should keep import refs (#28498) preventing us from upgrading to more recent versions of webpack ??? How so? afaik webpack doesn't use any const enums... (and if ti somehow did, they shouldn't be in their public API, as putting const enums in a public API is bad for compatibility)? — You are receiving this because you commented. Reply to this email directly, view it on GitHub<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fi.8713187.xyz%2Fmicrosoft%2FTypeScript%2Fpull%2F28498%3Femail_source%3Dnotifications%26email_token%3DAE633KJNI2TUMACBIUMJTN3PVGWIRA5CNFSM4GDLGXU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVJEIII%23issuecomment-491930657&data=02%7C01%7Cmiclo%40microsoft.com%7C9e1030c3f85349befc6008d6d7cf71c0%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636933683304198837&sdata=0fPxgLX%2BObc8VqZz9ijZB%2BTC%2FGzl%2BKzywJFIPcAq5RM%3D&reserved=0>, or mute the thread<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fi.8713187.xyz%2Fnotifications%2Funsubscribe-auth%2FAE633KOVKZTNTANWITXI2HLPVGWIRANCNFSM4GDLGXUQ&data=02%7C01%7Cmiclo%40microsoft.com%7C9e1030c3f85349befc6008d6d7cf71c0%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636933683304198837&sdata=TH%2BTa%2FAp4YelvBi2C8M9WJQM%2FKsAchdyLfvgzKlV4xI%3D&reserved=0>.

weswigham · 2019-05-13T20:08:39Z

The emitted JS

In webpack? They compile as TS now? Or in your project that consumes webpack's definition files?

MLoughry · 2019-05-13T20:38:51Z

No, the JS that Typescript emits from transpiling our project, which we then pass off to webpack to bundle.

// types.ts
export const enum Foo {
   Bar = 0,
   Baz = 1
};

export interface ViewState {
  foo: Foo;
}


// code.ts
import { Foo, ViewState } from './types';

export {Foo};

export const SomeViewState = {
  foo: Foo.Bar,
};

// EMITTED code.js
export { Foo };
export const SomeViewState = {
  foo: 0 /* Bar */,
}

Then, when that emitted code.js is passed to webpack for bundling, it throws an error saying that it could not find Foo to export in code.js

weswigham · 2019-05-13T21:45:52Z

Ahhhhhh ok, so not related to webpack at all really (well, webpack tosses the error since it does dependency analysis but it'd throw at runtime, too), got it. I was just worried that webpack had somehow adopted const enums, despite being a js project :P

MLoughry · 2019-05-13T21:55:38Z

Yeah. The strange thing is that webpack doesn't error on this on versions <=4.28.3, but it does for >=4.30.0 (I didn't bother narrowing down in-between).

weswigham · 2019-05-24T20:35:57Z

@typescript-bot test this
@typescript-bot run dt

typescript-bot · 2019-05-24T20:35:59Z

Heya @weswigham, I've started to run the extended test suite on this PR at 83893b3. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-05-24T20:35:59Z

Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 83893b3. You can monitor the build here. It should now contribute to this PR's status checks.

weswigham · 2019-06-11T20:43:38Z

@typescript-bot test this
@typescript-bot run dt

typescript-bot · 2019-06-11T20:43:41Z

Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 27bbfdb. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-06-11T20:43:41Z

Heya @weswigham, I've started to run the extended test suite on this PR at 27bbfdb. You can monitor the build here. It should now contribute to this PR's status checks.

weswigham · 2019-08-01T22:09:02Z

@typescript-bot test this
@typescript-bot run dt

typescript-bot · 2019-08-01T22:09:05Z

Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 219fbde. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-08-01T22:09:05Z

Heya @weswigham, I've started to run the extended test suite on this PR at 219fbde. You can monitor the build here. It should now contribute to this PR's status checks.

for exported const enums exported via export default Move some functionality around, small cleanup Remove unneeded const enum check

weswigham · 2019-08-01T22:21:46Z

@typescript-bot test this
@typescript-bot run dt
@typescript-bot user test this

typescript-bot · 2019-08-01T22:21:52Z

Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 9a9a2df. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-08-01T22:21:52Z

Heya @weswigham, I've started to run the parallelized community code test suite on this PR at 9a9a2df. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-08-01T22:21:52Z

Heya @weswigham, I've started to run the extended test suite on this PR at 9a9a2df. You can monitor the build here. It should now contribute to this PR's status checks.

…ort assignments

weswigham · 2019-08-01T23:59:15Z

@typescript-bot test this

typescript-bot · 2019-08-01T23:59:22Z

Heya @weswigham, I've started to run the extended test suite on this PR at b229333. You can monitor the build here. It should now contribute to this PR's status checks.

weswigham · 2019-08-02T00:53:32Z

reping @sheetalkamat and @sandersn for a review~

src/compiler/checker.ts

sandersn · 2019-08-06T22:49:29Z

Looks like this breaks dojo types. I'll file a bug for followup.

weswigham · 2019-08-06T23:12:00Z

@sandersn #32747 👍

weswigham requested review from sandersn and DanielRosenwasser November 13, 2018 02:46

sheetalkamat reviewed Nov 13, 2018

View reviewed changes

RyanCavanaugh assigned weswigham Jan 24, 2019

RyanCavanaugh assigned weswigham and unassigned weswigham Apr 30, 2019

weswigham force-pushed the preserve-const-export-assignment branch from 1924079 to 83893b3 Compare May 24, 2019 20:33

weswigham force-pushed the preserve-const-export-assignment branch from 83893b3 to 27bbfdb Compare June 11, 2019 20:43

weswigham force-pushed the preserve-const-export-assignment branch from 27bbfdb to 219fbde Compare August 1, 2019 22:07

Preserve const enums should keep import refs

9a9a2df

for exported const enums exported via export default Move some functionality around, small cleanup Remove unneeded const enum check

weswigham force-pushed the preserve-const-export-assignment branch from 219fbde to 9a9a2df Compare August 1, 2019 22:21

Only mark const enums as references with preserveConstEnums on in exp…

b229333

…ort assignments

sheetalkamat reviewed Aug 5, 2019

View reviewed changes

src/compiler/checker.ts Outdated Show resolved Hide resolved

weswigham added 2 commits August 5, 2019 14:25

Merge branch 'master' into preserve-const-export-assignment

b6d6d8b

Limit change to declarations and preserveConstEnums mode

692ab26

sheetalkamat approved these changes Aug 5, 2019

View reviewed changes

weswigham merged commit 3b54ffc into microsoft:master Aug 5, 2019

weswigham deleted the preserve-const-export-assignment branch August 5, 2019 23:47

sandersn mentioned this pull request Aug 6, 2019

dojo broken on typescript@next: Can't resolve members of merged namespace #32746

Closed

EvanCahill mentioned this pull request Aug 24, 2019

Cannot export const enum in Export Declaration #33060

Closed

Preserve const enums should keep import refs #28498

Preserve const enums should keep import refs #28498

Uh oh!

Conversation

weswigham commented Nov 13, 2018

Uh oh!

sheetalkamat Nov 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weswigham Nov 14, 2018

Choose a reason for hiding this comment

Uh oh!

MLoughry commented May 13, 2019

Uh oh!

weswigham commented May 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MLoughry commented May 13, 2019 via email

Uh oh!

weswigham commented May 13, 2019

Uh oh!

MLoughry commented May 13, 2019

Uh oh!

weswigham commented May 13, 2019

Uh oh!

MLoughry commented May 13, 2019

Uh oh!

weswigham commented May 24, 2019

Uh oh!

typescript-bot commented May 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented May 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weswigham commented Jun 11, 2019

Uh oh!

typescript-bot commented Jun 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Jun 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weswigham commented Aug 1, 2019

Uh oh!

typescript-bot commented Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weswigham commented Aug 1, 2019

Uh oh!

typescript-bot commented Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weswigham commented Aug 1, 2019

Uh oh!

typescript-bot commented Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weswigham commented Aug 2, 2019

Uh oh!

Uh oh!

sandersn commented Aug 6, 2019

Uh oh!

weswigham commented Aug 6, 2019

Uh oh!

Uh oh!

sheetalkamat Nov 13, 2018 •

edited

Loading

weswigham commented May 13, 2019 •

edited

Loading

typescript-bot commented May 24, 2019 •

edited

Loading

typescript-bot commented May 24, 2019 •

edited

Loading

typescript-bot commented Jun 11, 2019 •

edited

Loading

typescript-bot commented Jun 11, 2019 •

edited

Loading

typescript-bot commented Aug 1, 2019 •

edited

Loading

typescript-bot commented Aug 1, 2019 •

edited

Loading

typescript-bot commented Aug 1, 2019 •

edited

Loading

typescript-bot commented Aug 1, 2019 •

edited

Loading

typescript-bot commented Aug 1, 2019 •

edited

Loading

typescript-bot commented Aug 1, 2019 •

edited

Loading