Add flag to allow access to UMD globals from modules #30776

andrewbranch · 2019-04-05T18:34:29Z

A summary, since the issue discussion is quite long: the allowUmdGlobalAccess flag will allow accessing a global defined in a UMD module from anywhere, including from modules. This could potentially be unsafe since not all UMD modules define a global in the presence of a module system, but under some circumstances you really might need the ability. E.g. in a browser environment where one UMD module loads before require.js, that module will only be available as a global. So, this flag is an escape hatch for users stuck in scenarios like that.

src/compiler/commandLineParser.ts

src/compiler/checker.ts

tests/cases/conformance/externalModules/umd9.ts

RyanCavanaugh · 2019-04-05T22:03:49Z

Need to revert submodule updates, otherwise LGTM

weswigham · 2019-04-06T01:09:00Z

This flag quite literally just suppresses the error since we already "do the right thing" in the presence of the error with respect to types. Should we not just concede to community demand and add a generalized "suppressions": ["TS2686"]-type compiler option? Because that's all this new command is an alias for. (And I imagine if we do the more generalized option we can be smart and still return the errors as suggestions, which we do not (yet?) do here)

src/compiler/checker.ts

andrewbranch · 2019-04-06T01:23:55Z

@weswigham good point. I'm sure I'm missing some prior discussion here, but aren't there some error codes that are virtually unsuppressible, or would have horrifying consequences if suppressed? This particular error is a great example of something that would work, but I'm thinking about e.g. parser errors... suppressing a diagnostic from parseExpectedToken would essentially allow people to invent their own more concise non-standard syntax; in other cases, invalid syntax could parse as a totally different syntax tree than a user is expecting, without surfacing any diagnostics.

I guess a better summary of what I'm asking is—while I don't personally feel the need to try to force users into writing better TypeScript against their will, would a feature like that, on average, actually result in a better user experience? Or would it inadvertently wreak more havoc than solve problems?

As a newbie, this is a legitimate question, not rhetorical, and I'd happily defer to the team's consensus.

weswigham · 2019-04-06T01:36:35Z

or would have horrifying consequences if suppressed?

We already have // @ts-ignore comments that let you suppress anything, sooooooo

ajafff · 2019-04-06T06:14:37Z

We already have // @ts-ignore comments that let you suppress anything

IIRC that's not the case. It doesn't suppress parse errors and declaration emit diagnostics (don't know about binder diagnostics)

weswigham · 2019-04-06T08:21:29Z

It doesn't suppress parse errors and declaration emit diagnostics (don't know about binder diagnostics)

We can exclude those from the errors we'll suppress, too, then. I'm just thinking that adding a suppressions argument that acts like a ts-ignore for all errors of a specific type is probably more general, and I know I've seen requests for it (much as I personally loathe the idea of suppressing any of our errors).

… set

sheetalkamat · 2019-04-11T16:06:02Z

src/compiler/checker.ts

@@ -1635,7 +1635,7 @@ namespace ts {
                if (result && isInExternalModule && (meaning & SymbolFlags.Value) === SymbolFlags.Value && !(originalLocation!.flags & NodeFlags.JSDoc)) {
                    const merged = getMergedSymbol(result);
                    if (length(merged.declarations) && every(merged.declarations, d => isNamespaceExportDeclaration(d) || isSourceFile(d) && !!d.symbol.globalExports)) {
-                        error(errorLocation!, Diagnostics._0_refers_to_a_UMD_global_but_the_current_file_is_a_module_Consider_adding_an_import_instead, unescapeLeadingUnderscores(name)); // TODO: GH#18217
+                        errorOrSuggestion(!compilerOptions.allowUmdGlobalAccess, errorLocation!, Diagnostics._0_refers_to_a_UMD_global_but_the_current_file_is_a_module_Consider_adding_an_import_instead, unescapeLeadingUnderscores(name));


Should this diagnostic change to additionally mention or enable allowUmdGlobalAccess if?

Since this is errorOrSuggestion based on the compiler option, we would have to use two different messages depending on the value of the option (since users with it enabled should not see a message that says "Enable the 'allowUmdGlobalAccess' compiler option")... do you think that's worth it?

I'd leave as-is. In general for errors we think are usually correct, we try not to mention commandline flags to disable the error, since people will often try disabling the error before even checking if their code works or not

mihailik · 2019-04-23T12:27:28Z

tests/cases/conformance/externalModules/umd9.ts

+export as namespace Foo;
+
+// @filename: a.ts
+/// <reference path="foo.d.ts" />


Is this a requirement in this file?

Given the original issue #1078 is sooo convoluted, it may be well worth a bit of write-up how this new feature works.

P.S. I have a feeling it may be extremely useful in some scenarios I am involved, but need a more defined behaviour/usage/spec to know for sure.

You're right, the original issue is a lot to absorb, but this “feature” is actually quite simple. All it does is suppress a particular error message. This one:

The triple slash reference isn't related to this feature at all; it's related to the @noImplicitReferences option which is just a configuration for our test harness.

Does that clear things up, or do you have any additional specific questions?

Thanks, all clear.

It's awesome: from complex to trivial in a couple sentences.

tqma113

LGTM, That's great.

andrewbranch added the Add a Flag Any problem can be solved by flags, except for the problem of having too many flags label Apr 5, 2019

andrewbranch requested review from DanielRosenwasser, sandersn and sheetalkamat April 5, 2019 18:57

sheetalkamat requested changes Apr 5, 2019

View reviewed changes

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

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

tests/cases/conformance/externalModules/umd9.ts Show resolved Hide resolved

andrewbranch requested a review from sheetalkamat April 5, 2019 21:37

weswigham reviewed Apr 6, 2019

View reviewed changes

src/compiler/checker.ts Show resolved Hide resolved

DanielRosenwasser removed the Add a Flag Any problem can be solved by flags, except for the problem of having too many flags label Apr 8, 2019

andrewbranch added 8 commits April 8, 2019 09:46

Add allowUmdGlobalAccess flag

2ee93bf

Add test

6d1d680

Don’t emit diagnostic accessing UMD globals from modules when flag is…

11e4516

… set

Add baselines for UMD global access

57b9e38

Add period to compiler flag description to match others

28b21df

Update baselines with new flag

6c06507

Add affectsSemanticDiagnostics to flag (and move to right spot)

98cd89f

Use suggestion instead of ignoring UMD global access

786753d

andrewbranch force-pushed the feature/10178 branch from 1647187 to 786753d Compare April 8, 2019 16:47

RyanCavanaugh approved these changes Apr 9, 2019

View reviewed changes

sheetalkamat reviewed Apr 11, 2019

View reviewed changes

andrewbranch merged commit b6a0988 into microsoft:master Apr 19, 2019

andrewbranch deleted the feature/10178 branch April 19, 2019 01:05

mihailik reviewed Apr 23, 2019

View reviewed changes

andrewbranch mentioned this pull request May 23, 2019

Add new TypeScript compiler option 'allowUmdGlobalAccess' to tsconfig and jsconfig SchemaStore/schemastore#699

Merged

tqma113 reviewed Sep 30, 2019

View reviewed changes

shobhitsinghal624 mentioned this pull request May 7, 2020

[tsserver 2686] [I] '_' refers to a UMD global, but the current file is a module. neoclide/coc-tsserver#149

Closed

taoliujun mentioned this pull request Nov 27, 2023

关于tsconfig.json，最后更新2023/11月... taoliujun/blog#23

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add flag to allow access to UMD globals from modules #30776

Add flag to allow access to UMD globals from modules #30776

andrewbranch commented Apr 5, 2019

RyanCavanaugh commented Apr 5, 2019

weswigham commented Apr 6, 2019 •

edited

Loading

andrewbranch commented Apr 6, 2019

weswigham commented Apr 6, 2019

ajafff commented Apr 6, 2019

weswigham commented Apr 6, 2019

sheetalkamat Apr 11, 2019

andrewbranch Apr 18, 2019

RyanCavanaugh Apr 18, 2019

mihailik Apr 23, 2019

andrewbranch Apr 24, 2019

mihailik Apr 25, 2019

tqma113 left a comment •

edited

Loading

Add flag to allow access to UMD globals from modules #30776

Add flag to allow access to UMD globals from modules #30776

Conversation

andrewbranch commented Apr 5, 2019

RyanCavanaugh commented Apr 5, 2019

weswigham commented Apr 6, 2019 • edited Loading

andrewbranch commented Apr 6, 2019

weswigham commented Apr 6, 2019

ajafff commented Apr 6, 2019

weswigham commented Apr 6, 2019

sheetalkamat Apr 11, 2019

Choose a reason for hiding this comment

andrewbranch Apr 18, 2019

Choose a reason for hiding this comment

RyanCavanaugh Apr 18, 2019

Choose a reason for hiding this comment

mihailik Apr 23, 2019

Choose a reason for hiding this comment

andrewbranch Apr 24, 2019

Choose a reason for hiding this comment

mihailik Apr 25, 2019

Choose a reason for hiding this comment

tqma113 left a comment • edited Loading

Choose a reason for hiding this comment

weswigham commented Apr 6, 2019 •

edited

Loading

tqma113 left a comment •

edited

Loading