Skip to content
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

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

Merged
merged 8 commits into from
Apr 19, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
andrewbranch marked this conversation as resolved.
Show resolved Hide resolved
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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
}
Expand Down
7 changes: 7 additions & 0 deletions src/compiler/commandLineParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -575,6 +575,13 @@ namespace ts {
category: Diagnostics.Module_Resolution_Options,
description: Diagnostics.Do_not_resolve_the_real_path_of_symlinks,
},
{
name: "allowUmdGlobalAccess",
type: "boolean",
affectsSemanticDiagnostics: true,
category: Diagnostics.Module_Resolution_Options,
description: Diagnostics.Allow_accessing_UMD_globals_from_modules,
},

// Source Maps
{
Expand Down
4 changes: 4 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -4938,5 +4938,9 @@
"Convert parameters to destructured object": {
"category": "Message",
"code": 95075
},
"Allow accessing UMD globals from modules.": {
"category": "Message",
"code": 95076
}
}
1 change: 1 addition & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4580,6 +4580,7 @@ namespace ts {
allowJs?: boolean;
/*@internal*/ allowNonTsExtensions?: boolean;
allowSyntheticDefaultImports?: boolean;
allowUmdGlobalAccess?: boolean;
allowUnreachableCode?: boolean;
allowUnusedLabels?: boolean;
alwaysStrict?: boolean; // Always combine with strict property
Expand Down
1 change: 1 addition & 0 deletions tests/baselines/reference/api/tsserverlibrary.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2489,6 +2489,7 @@ declare namespace ts {
interface CompilerOptions {
allowJs?: boolean;
allowSyntheticDefaultImports?: boolean;
allowUmdGlobalAccess?: boolean;
allowUnreachableCode?: boolean;
allowUnusedLabels?: boolean;
alwaysStrict?: boolean;
Expand Down
1 change: 1 addition & 0 deletions tests/baselines/reference/api/typescript.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2489,6 +2489,7 @@ declare namespace ts {
interface CompilerOptions {
allowJs?: boolean;
allowSyntheticDefaultImports?: boolean;
allowUmdGlobalAccess?: boolean;
allowUnreachableCode?: boolean;
allowUnusedLabels?: boolean;
alwaysStrict?: boolean;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"compilerOptions": {
"allowUmdGlobalAccess": true
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
// "allowSyntheticDefaultImports": true, /* Allow default imports from modules with no default export. This does not affect code emit, just typechecking. */
"esModuleInterop": true /* Enables emit interoperability between CommonJS and ES Modules via creation of namespace objects for all imports. Implies 'allowSyntheticDefaultImports'. */
// "preserveSymlinks": true, /* Do not resolve the real path of symlinks. */
// "allowUmdGlobalAccess": true, /* Allow accessing UMD globals from modules. */

/* Source Map Options */
// "sourceRoot": "", /* Specify the location where debugger should locate TypeScript files instead of source locations. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
// "allowSyntheticDefaultImports": true, /* Allow default imports from modules with no default export. This does not affect code emit, just typechecking. */
"esModuleInterop": true, /* Enables emit interoperability between CommonJS and ES Modules via creation of namespace objects for all imports. Implies 'allowSyntheticDefaultImports'. */
// "preserveSymlinks": true, /* Do not resolve the real path of symlinks. */
// "allowUmdGlobalAccess": true, /* Allow accessing UMD globals from modules. */

/* Source Map Options */
// "sourceRoot": "", /* Specify the location where debugger should locate TypeScript files instead of source locations. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
// "allowSyntheticDefaultImports": true, /* Allow default imports from modules with no default export. This does not affect code emit, just typechecking. */
"esModuleInterop": true /* Enables emit interoperability between CommonJS and ES Modules via creation of namespace objects for all imports. Implies 'allowSyntheticDefaultImports'. */
// "preserveSymlinks": true, /* Do not resolve the real path of symlinks. */
// "allowUmdGlobalAccess": true, /* Allow accessing UMD globals from modules. */

/* Source Map Options */
// "sourceRoot": "", /* Specify the location where debugger should locate TypeScript files instead of source locations. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
// "allowSyntheticDefaultImports": true, /* Allow default imports from modules with no default export. This does not affect code emit, just typechecking. */
"esModuleInterop": true /* Enables emit interoperability between CommonJS and ES Modules via creation of namespace objects for all imports. Implies 'allowSyntheticDefaultImports'. */
// "preserveSymlinks": true, /* Do not resolve the real path of symlinks. */
// "allowUmdGlobalAccess": true, /* Allow accessing UMD globals from modules. */

/* Source Map Options */
// "sourceRoot": "", /* Specify the location where debugger should locate TypeScript files instead of source locations. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
// "allowSyntheticDefaultImports": true, /* Allow default imports from modules with no default export. This does not affect code emit, just typechecking. */
"esModuleInterop": true /* Enables emit interoperability between CommonJS and ES Modules via creation of namespace objects for all imports. Implies 'allowSyntheticDefaultImports'. */
// "preserveSymlinks": true, /* Do not resolve the real path of symlinks. */
// "allowUmdGlobalAccess": true, /* Allow accessing UMD globals from modules. */

/* Source Map Options */
// "sourceRoot": "", /* Specify the location where debugger should locate TypeScript files instead of source locations. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
// "allowSyntheticDefaultImports": true, /* Allow default imports from modules with no default export. This does not affect code emit, just typechecking. */
"esModuleInterop": true /* Enables emit interoperability between CommonJS and ES Modules via creation of namespace objects for all imports. Implies 'allowSyntheticDefaultImports'. */
// "preserveSymlinks": true, /* Do not resolve the real path of symlinks. */
// "allowUmdGlobalAccess": true, /* Allow accessing UMD globals from modules. */

/* Source Map Options */
// "sourceRoot": "", /* Specify the location where debugger should locate TypeScript files instead of source locations. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
// "allowSyntheticDefaultImports": true, /* Allow default imports from modules with no default export. This does not affect code emit, just typechecking. */
"esModuleInterop": true /* Enables emit interoperability between CommonJS and ES Modules via creation of namespace objects for all imports. Implies 'allowSyntheticDefaultImports'. */
// "preserveSymlinks": true, /* Do not resolve the real path of symlinks. */
// "allowUmdGlobalAccess": true, /* Allow accessing UMD globals from modules. */

/* Source Map Options */
// "sourceRoot": "", /* Specify the location where debugger should locate TypeScript files instead of source locations. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
// "allowSyntheticDefaultImports": true, /* Allow default imports from modules with no default export. This does not affect code emit, just typechecking. */
"esModuleInterop": true /* Enables emit interoperability between CommonJS and ES Modules via creation of namespace objects for all imports. Implies 'allowSyntheticDefaultImports'. */
// "preserveSymlinks": true, /* Do not resolve the real path of symlinks. */
// "allowUmdGlobalAccess": true, /* Allow accessing UMD globals from modules. */

/* Source Map Options */
// "sourceRoot": "", /* Specify the location where debugger should locate TypeScript files instead of source locations. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
// "allowSyntheticDefaultImports": true, /* Allow default imports from modules with no default export. This does not affect code emit, just typechecking. */
"esModuleInterop": true /* Enables emit interoperability between CommonJS and ES Modules via creation of namespace objects for all imports. Implies 'allowSyntheticDefaultImports'. */
// "preserveSymlinks": true, /* Do not resolve the real path of symlinks. */
// "allowUmdGlobalAccess": true, /* Allow accessing UMD globals from modules. */

/* Source Map Options */
// "sourceRoot": "", /* Specify the location where debugger should locate TypeScript files instead of source locations. */
Expand Down
19 changes: 19 additions & 0 deletions tests/baselines/reference/umd9.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
//// [tests/cases/conformance/externalModules/umd9.ts] ////

//// [foo.d.ts]
declare class Thing {
foo(): number;
}
export = Thing;
export as namespace Foo;

//// [a.ts]
/// <reference path="foo.d.ts" />
export const x = Foo; // OK in value position because allowUmdGlobalAccess: true


//// [a.js]
"use strict";
exports.__esModule = true;
/// <reference path="foo.d.ts" />
exports.x = Foo; // OK in value position because allowUmdGlobalAccess: true
19 changes: 19 additions & 0 deletions tests/baselines/reference/umd9.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
=== tests/cases/conformance/externalModules/a.ts ===
/// <reference path="foo.d.ts" />
export const x = Foo; // OK in value position because allowUmdGlobalAccess: true
>x : Symbol(x, Decl(a.ts, 1, 12))
>Foo : Symbol(Foo, Decl(foo.d.ts, 3, 15))

=== tests/cases/conformance/externalModules/foo.d.ts ===
declare class Thing {
>Thing : Symbol(Thing, Decl(foo.d.ts, 0, 0))

foo(): number;
>foo : Symbol(Thing.foo, Decl(foo.d.ts, 0, 21))
}
export = Thing;
>Thing : Symbol(Thing, Decl(foo.d.ts, 0, 0))

export as namespace Foo;
>Foo : Symbol(Foo, Decl(foo.d.ts, 3, 15))

19 changes: 19 additions & 0 deletions tests/baselines/reference/umd9.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
=== tests/cases/conformance/externalModules/a.ts ===
/// <reference path="foo.d.ts" />
export const x = Foo; // OK in value position because allowUmdGlobalAccess: true
>x : typeof Thing
>Foo : typeof Thing

=== tests/cases/conformance/externalModules/foo.d.ts ===
declare class Thing {
>Thing : Thing

foo(): number;
>foo : () => number
}
export = Thing;
>Thing : Thing

export as namespace Foo;
>Foo : typeof Thing

14 changes: 14 additions & 0 deletions tests/cases/conformance/externalModules/umd9.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// @module: commonjs
andrewbranch marked this conversation as resolved.
Show resolved Hide resolved
// @noImplicitReferences: true
// @allowUmdGlobalAccess: true

// @filename: foo.d.ts
declare class Thing {
foo(): number;
}
export = Thing;
export as namespace Foo;

// @filename: a.ts
/// <reference path="foo.d.ts" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

image

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, all clear.

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

export const x = Foo; // OK in value position because allowUmdGlobalAccess: true