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

Consider allowing access to UMD globals from modules #10178

Closed
RyanCavanaugh opened this issue Aug 5, 2016 · 73 comments · Fixed by #30776
Closed

Consider allowing access to UMD globals from modules #10178

RyanCavanaugh opened this issue Aug 5, 2016 · 73 comments · Fixed by #30776
Assignees
Labels
Add a Flag Any problem can be solved by flags, except for the problem of having too many flags Committed The team has roadmapped this issue Good First Issue Well scoped, documented and has the green light Help Wanted You can do this Suggestion An idea for TypeScript

Comments

@RyanCavanaugh
Copy link
Member

Feedback from #7125 is that some people actually do mix and match global and imported UMD libraries, which we didn't consider to be a likely scenario when implementing the feature.

Three plausible options:

  1. Do what we do today - bad because there's no good workaround
  2. Allow some syntax or configuration to say "this UMD global is actually available globally" - somewhat complex, but doable
  3. Allow access to all UMD globals regardless of context - misses errors where people forget to import the UMD global in a file. These are presumably somewhat rare, but it would be dumb to miss them

Sounds like it would work but probably wouldn't:

  1. Flag imported UMD modules as "not available for global" - bad because UMD modules will be imported in declaration files during module augmentation. It'd be weird to have differing behavior of imports from implementation files vs declaration files.

I'm inclined toward option 3 for simplicity's sake, but could see option 2 if there's some reasonably good syntax or configuration we could use in a logical place. Detecting the use of a UMD global in a TSLint rule would be straightforward if someone wanted to do this.

One path forward would be to implement option 3 and if it turns out people make the "forgot to import" error often, add a tsconfig option globals: [] that explicitly specifies which UMD globals are allowed.

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Aug 5, 2016
@RyanCavanaugh RyanCavanaugh changed the title Consider allowing access to UMD globals Consider allowing access to UMD globals from modules Aug 5, 2016
@speigg
Copy link

speigg commented Aug 5, 2016

Instead of allowing access to all UMD globals regardless of context, wouldn't it be simpler to only allow access to the UMD global if the UMD module has been explicitly "referenced" (not imported) via either the ///<reference types=<>> syntax, or via the types configuration in tsconfig.json ?

@speigg
Copy link

speigg commented Aug 5, 2016

In other words, why not just allow ///<reference types=<>> to be used within a module?

@RyanCavanaugh
Copy link
Member Author

If we said /// <reference type="x" /> meant that x was globally available everywhere, it's frequently going to be the case that some .d.ts file somewhere is going to incorrectly reference things that aren't really global (I can tell you this because I've been maintaning the 2.0 branch of DefinitelyTyped and it's an extremely common error).

Conversely if it's only available in that file, then you're going to have to be copying and pasting reference directives all over the place, which is really annoying. These directives are normally idempotent so introducing file-specific behavior is weird.

@speigg
Copy link

speigg commented Aug 5, 2016

I see. Well, if this isn't affecting anyone else, perhaps it's better to maintain the current behavior. I'll just have to transition fully towards importing things as modules.

Edit: glad to see this does affect many people besides myself.

@RyanCavanaugh RyanCavanaugh added Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature and removed In Discussion Not yet reached consensus labels Aug 22, 2016
@RyanCavanaugh
Copy link
Member Author

Current theory is to just ask people to migrate to using modules or not. If other people run into this, please leave a comment with exactly which libraries you were using so we can investigate more.

@ShawnTalbert
Copy link

I am using lodash, which does not come with typings of its own. I also have a situation where in my runtime environment it's easiest to use relative path import statements. So, I have a combination of import statements with local relative paths and folder relatives ('./foo' as well as 'N/bar').

If I manually copy the @types/lodash/index.d.ts to node_modules/lodash/ I can get things to typecheck.

Up till now my workaround was using ///<amd-dependency path='../lodash' name="_"> (and no import statement). With this combo, the @types/lodash definitions would be seen 'globally' by the compiler and still have the correct relative path ( ../lodash ) in the emitted JS.

I hope this is a scenario close enough to this issue?

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Nov 13, 2016

Can you clarify

I also have a situation where in my runtime environment it's easiest to use relative path import statements.

and

If I manually copy the @types/lodash/index.d.ts to node_modules/lodash/ I can get things to typecheck.

please? I'm not familiar with this scenario, so what is the purpose of this and why is it helpful?

@d-ph
Copy link

d-ph commented Nov 19, 2016

Hi guys,

I'm in the process of looking for a solution to current @types/bluebird declaration problem (please don't spend time reading it). I found, that the problem could be solved, by adding export as namespace Promise; to the .d.ts, but then I run into the problem described by this github issue.

In short, I'd like the following to work:

  1. git clone -b vanilla-es5-umd-restriction-problem https://github.com/d-ph/typescript-bluebird-as-global-promise.git
  2. cd typescript-bluebird-as-global-promise
  3. npm install
  4. Edit node_modules/@types/bluebird/index.d.ts by adding export as namespace Promise; above the export = Bluebird; line.
  5. npm run tsc

Current result:
A couple of 'Promise' refers to a UMD global, but the current file is a module. Consider adding an import instead. errors.

Expected result:
Compilation succeeds.

This problem is particularly difficult, because it's triggered by Promise usage in both dev's code and 3rd party code (RxJS in that case). The latter assumes that Promise is global (provided by JS standard), therefore will never change to using e.g. import Promise from std; // (not that "std" is a thing).

I'd really appreciate a way to use UMD modules as both importable modules and as globals.

Thanks.

----------------------------- Update

I ended up solving this issue differently (namely: by Promise and PromiseConstructor interfaces augmentation).

@aluanhaddad
Copy link
Contributor

The "globals": [] tsconfig option seems preferable to making them visible everywhere. With UMD declaration becoming the norm, the odds of accidentally forgetting to import a module are high. Please consider retaining the current behavior. This should be an error.

Anecdotally I remember when moment removed their global window.moment variable in a point release. We thought we had been judiciously importing it everywhere, but we had forgotten about 5 places.

Of course a UMD package will be available in the global scope at runtime, but when it becomes available depends on the order in which other modules are loaded.

@billti
Copy link
Member

billti commented Nov 25, 2016

+1 on this. I was just trying to use React with SystemJS, and as React doesn't bundle very well, I'm just loading it straight from the CDN in a script tag, and thus the React/ReactDOM objects are available globally.

I'm writing code as modules as best practice, but this will be bundled (Rollup) into one runtime script that executes on load. It's a pain (and a lie) to have to import from react/react-dom, and then configure the loader to say "not really, these are globals" (similar to the example WebPack configuration given in https://www.typescriptlang.org/docs/handbook/react-&-webpack.html ). It would be much easier (and more accurate) to simply have these available as globals in my modules. The steps I tried, as they seemed intuitive, were:

  1. npm install --save-dev @types/react @types/react-dom
  2. In my tsconfig.json: "jsx": "react", "types": ["react", "react-dom"]
  3. In my module: export function MyComponent() { return <div>{"Hello, world"}</div>; }
  4. Similarly: ReactDOM.render(...)

However this results in the error React refers to a UMD global, but the current file is a module. Consider adding an import instead.

If this just worked, this would be far simpler than pretending in the code it's a module, then configuring the loader/bundler that it's not. (Or alternatively, I kinda got it to do what I expected by adding a file containing the below. Now my modules can use React & ReactDOM as globals without error, but it's kinda ugly/hacky - though there may be a simpler way I've missed):

import * as ReactObj from "react";
import * as ReactDOMObj from "react-dom";

declare global {
    var React: typeof ReactObj;
    var ReactDOM: typeof ReactDOMObj;
}

@kenisteward
Copy link

I also agree with options three plus globals: [] backup. That seems pretty intuitive to new and old users and would give the exact functionality that people need.

I'm not a specialist on the code so I can't really say if 2 would be more preferable or not but I think it would also be intuitive given the configuration for it is straightforward.

If I wanted to look into help implementing any of these where should I go?

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Nov 29, 2016

This really should be behind a flag. It is a massive refactoring hazard. Even if the flag is specified true by default. I think this has to continue to work in the original scenario otherwise we're losing the primary benefit of UMD declarations.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Nov 29, 2016

React doesn't bundle very well

@billti can you elaborate?

It's a pain (and a lie) to have to import from react/react-dom, and then configure the loader to say "not really, these are globals"

The only reason I wrote that in the tutorial is because using externals cuts down on bundle-time. If you use the global React variable without importing, you can't easily switch to modules later on, whereas imports give you the flexibility of using either given your loader.

@billti
Copy link
Member

billti commented Nov 29, 2016

See this issue (rollup/rollup#855) for one example on how they're trying to optimize bundling and the sizes observed. Effectively in my setup (using Rollup) I saw minimal size gains bundling React, so I'd rather just serve it from a CDN. To me that has the benefits of:

a) Less requests (and bandwidth) to my site.
b) Less time taken to bundle in my build chain.
c) Less code to re-download on the client every time I push a change (as only my code is in the bundle that gets re-downloaded, and React is still in the client cache unmodified - thus getting 304s).

Looking in the Chrome Dev Tools on loading the site, React and React-dom (the minified versions), on a GZipped HTTP connection, are only 47kb of network traffic, which is less than most images on a site, so I'm not worried about trying to reduce that much anyway, unless there's really big gains to be had (e.g. 50% reduction).

@billti
Copy link
Member

billti commented Nov 29, 2016

As an addendum: I'd also note that without this option, you are also forcing folks to use a bundler that elides these imports, as the TypeScript compiler itself has no configuration for saying "this module is really a global", and will thus emit imports (or requires, or defines) for modules which wouldn't resolve at runtime.

@aluanhaddad
Copy link
Contributor

@billti SystemJS has full support for this scenario. You can swap between using a locally installed package during development and using a CDN in production. It also has full support for metadata which specifies that all imports should actually indicate references to a global variable that will be fetched once and attached to the window object when first needed, in production this can come from a CDN. I haven't done this with react but I have done it with angular 1.x

@billti
Copy link
Member

billti commented Nov 29, 2016

Thanks @aluanhaddad . Interesting... I was actually trying to get something similar working that led me to this roadblock, and couldn't figure it out, so just this morning asked the question on the SystemJS repo. If you can answer how to achieve systemjs/systemjs#1510 , that'd be really helpful :-)

Note: My other comment still stands, that the emit by TypeScript itself is not usable without this, as you need something like SystemJS/WebPack/Rollup etc... to map the imports to globals for the code to run.

@aluanhaddad
Copy link
Contributor

I'll take a look and see if I can make a working example, I haven't done it in quite a while and I don't have access to the source code that I had at the time but I'm hundred percent sure it's possible.

To your second point, that's exactly what SystemJS does. It will map those imports to the global and understands that the global is actually being requested and has already been loaded. The output is definitely usable

@billti
Copy link
Member

billti commented Nov 30, 2016

FYI: I got this working in SystemJS using the SystemJS API and added my solution on systemjs/systemjs#1510 . Thanks.

Re my second point: Yes, I know that's exactly what the loaders can do. That's my point, they can map an imported module to a global, but TypeScript can't - so you have to use a loader to make your code valid at runtime. So it's a catch-22 with this original issue, where you can't declare that the global (in this case React) is available in the module, you have to import it as if it were a module (which it isn't).

@DanielRosenwasser
Copy link
Member

My other comment still stands, that the emit by TypeScript itself is not usable without this, as you need something like SystemJS/WebPack/Rollup etc... to map the imports to globals for the code to run.

@billti I don't understand. What is a scenario in which your only option is to use the global version of a module, but TypeScript doesn't allow you to do that? I've only seen scenarios where a library is available as both a global and a module.

@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this Committed The team has roadmapped this issue Good First Issue Well scoped, documented and has the green light and removed In Discussion Not yet reached consensus labels Aug 23, 2018
@RyanCavanaugh RyanCavanaugh added this to the Community milestone Aug 23, 2018
@RyanCavanaugh
Copy link
Member Author

Accepting PRs for a new flag that allows access to UMD globals from all modules.

Implementation-wise, it's quite straightforward... but we do need to name it. We kicked around a dozen terrible names at the suggestion review meeting and hated all of them, so it's up to y'all to come up with something palatable. Please halp.

@saschanaz
Copy link
Contributor

saschanaz commented Aug 23, 2018

We kicked around a dozen terrible names at the suggestion review meeting and hated all of them

What were they?

so it's up to y'all to come up with something palatable.

Maybe umdUseGlobal or something.

@FranklinWhale
Copy link

FranklinWhale commented Aug 23, 2018

I would suggest importAllNamespaces for the name of UMD global flag because UMD globals are usually export as namespace.

@RyanCavanaugh Did the team discuss about the type issue?

@RyanCavanaugh
Copy link
Member Author

@FranklinWhale yes.

@cailie
Copy link

cailie commented Oct 2, 2018

@saschanaz Asked this already, but I'm also curious... @RyanCavanaugh Do you remember what terrible names were discussed?

@RyanCavanaugh
Copy link
Member Author

I think the chain went something like this

  • allowUmdGlobalAccessFromModules - most precise but soooooo long
  • assumeGlobalUmd - ugh
  • allowModuleUmdGlobals - "globals" ??
  • umdAlwaysGlobal - 🤢
  • allowUmdGlobals - but I already can?
  • allowUmdGlobalAccess - skips the module part but probably no one cares?

I would choose the last one if forced to

@cailie
Copy link

cailie commented Oct 2, 2018

Thank you!

I like allowUmdGlobalAccessFromModules the best because, although it is long, its precision makes it easiest to remember. I would think, "What's that option that allows UMD globals to be accessed from modules? Oh yeah, it's allowUmdGlobalAccessFromModules, of course!"

Using the prefix "allow" matches the naming convention of other options, which is good.

Plus... there are other options that are about as long :)

allowUmdGlobalAccessFromModules : 31 characters

allowSyntheticDefaultImports : 28 characters
strictPropertyInitialization : 28 characters
suppressExcessPropertyErrors : 28 characters
suppressImplicitAnyIndexErrors : 30 characters
forceConsistentCasingInFileNames : 32 characters

@Gilead
Copy link

Gilead commented Dec 10, 2018

What is the current workaround? I have been googling for the past hour and can't find any workable solutions.
I'd rather not cast to 'any' or downgrade to a working Typescript version but can't find any other options.
Is there an experimental build somewhere that has a compiler flag that fixes this issue?
(by the way, 'allowUmdGlobalAccessFromModules' is an excellent name; it's not like we'd be typing it 50 times a day :-) )

We're using tsc 3.2.2 with lodash statically included in the top HTML file; with require.js; d.ts obtained from latest DefinitelyTyped; example code that fails to compile:

/// <reference path="..." />

class Example<T extends IThingWithTitle<T>> {

    public test = (arg : T[]) : void => {
        _.sortBy(arg, (el : T) => { return el.title; }); // TS2686: '_' refers to a UMD global, but the current file is a module. Consider adding an import instead.
    };

}

export = Example;

(please don't tell me I need to turn the project upside down, I know we're behind the curve in some aspects)

Update: ((window)._)/* FIXME #10178 */.sortBy(...) works but dear Lord, it is ugly :-P

@adanski
Copy link

adanski commented Dec 15, 2018

@Gilead, solution from this comment works just fine for now: #10178 (comment)

@simonhaenisch
Copy link

Is there any progress on this? I have a case where the mentioned work-around doesn't seem to work (using typescript@3.2.2) because I'm running into this issue.


First I tried this:

import 'firebase';

declare global {
  const firebase;
}

This implicitly gives the global firebase the type any, and then applies the namespace (with the same name) to it. First this seemed to work because it shows the proper tooltips/intellisense for all the top-level keys of firebase.

However it doesn't actually work (I assume because it then switches to using it as type any, which might be a bug?):


So I tried the workaround mentioned here without success (it works for others though):

import _firebase from 'firebase'; // same with = require('firebase') 

declare global {
  const firebase: typeof _firebase;
}

=> 'firebase' is referenced directly or indirectly in its own type annotation. [2502]
(even though the namespace is aliased?)


I also tried

import * as _firebase from 'firebase';

declare global {
  const firebase: typeof _firebase;
}

=> Circular definition of import alias '_firebase'. [2303]
(maybe because of export = firebase; export as namespace firebase; in its definition?)


And finally, if I just do the import 'firebase', I'm back at

'firebase' refers to a UMD global, but the current file is a module. Consider adding an import instead. [2686]


If anyone has a solution for this, it would be very appreciated. Otherwise any of the suggestions to solve this that were mentioned so far seems fine to me really (flag, triple slash reference, types in tsconfig, having a global or external object in tsconfig).

Re @aluanhaddad's comment

This can't let all this have been in vein and go back to globals!

I'm not trying to go back to globals, I'm just trying to have a couple heavy dependencies load separately from my app bundle, while still using modules for everything else, because it brings a few benefits: the dependencies can be cached properly because they don't get updated as often as my app bundle; my bundle size doesn't explode (sometimes because of code-splitting the bundler includes the same dependency multiple times), which means less downloading for my app users; rebuilding my bundles during dev is way faster.

@RyanCavanaugh
Copy link
Member Author

This is really a very easy feature to add; it'd be great for a community member to take it up.

@simonhaenisch
Copy link

@RyanCavanaugh I looked into it and kind of figured out I have to add the option to compiler/types.ts and modify the umd global check in compiler/checker.ts, and i think I need to add it to compiler/commandLineParser.ts as well... but I think it would take me quite a while to get it done because I'm not familiar with the source at all (like how do I add a description for the CLI flag without breaking the i18n). For now I'm going to wait for someone else who knows the source already to take it on.

@fightingcat
Copy link

fightingcat commented Feb 3, 2019

@simonhaenisch You can declare it in a non-module declaration file, and to avoid of circular reference, you can re-export it in another UMD module declaration. The original firebase is declared as a namespace, lucky for us, it will augment our declaration, not causing error.

// umd.d.ts
import firebase = require("firebase");
export import firebase = firebase;
export as namespace UMD;

// global.d.ts
declare const firebase: typeof UMD.firebase;

Unfortunately, what we declared is a value, not a namespace, so you can't do something like let x: firebase.SomeInterface, the only way to alias a namespace is by declaring an import, but you can't declare import firebase = UMD.firebase;, because namespace won't augment it. Sure we can use a different name for only namspace using in type, but that will cause confusion, I'd rather drop the rest of the code I talked above, assign it to a global value at runtime, make the import alias really work.

@RyanCavanaugh RyanCavanaugh modified the milestones: Community, Backlog Mar 7, 2019
@esc-mhild
Copy link

Similar to the previous comment, we are lazy-loading hls.js (UMD) and reference types thus:

In hls.d.ts:

import * as Hls from 'hls.js';
declare global {
    const Hls: typeof Hls;
}

In the .ts file using the lazy-loaded UMD module:

/// <reference path="hls.d.ts" />
// now use it
if(Hls.isSupported()){
 ...
} 

Tested in Typescript >= 3.0.1 and 3.4.1.

Rationale is incomplete browser support for dynamic module imports.

@trusktr
Copy link
Contributor

trusktr commented May 13, 2019

@MatthiasHild Can it be done without the /// <reference path="hls.d.ts" /> comment?

EDIT, yes it can, as long as the declaration is within an included .d.ts file that does NOT have the same name as another .ts file, based on this SO question (that's what was getting me and why I asked).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Add a Flag Any problem can be solved by flags, except for the problem of having too many flags Committed The team has roadmapped this issue Good First Issue Well scoped, documented and has the green light Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.