Closed yortus closed 8 years ago
@bterlson should Edge have errored on assigning to Promise, on executing the asynchronous function, or is it exhibiting correct behavior?
@DanielRosenwasser this seems to have been asked on tc39/ecmascript-asyncawait here, and answered in the comment thread for that issue by @domenic and @bterlson here and here.
That issue talks about replacing/modifying global.Promise
, which is legal but does not alter the fact that async
functions still return an instance of the intrinsic %Promise%.
If changing global.Promise
cannot change the return type of async
functions, it seems logical that having a variable called Promise
in local scope certainly won't change in either.
Ideally transpilers should do something like this in a "header" file that runs before any author code:
const $$typeScriptRuntime = {
Promise: global.Promise,
PromiseResolve: global.Promise.resolve,
PromiseThen: global.Promise.prototype.then
};
then when they actually use it, instead of just straight transpiling to Promise
or Promise.resolve
or .then
, they can use these stored and unchangeable values.
(Also the TypeScript transpiler's "cast" function seems entirely unnecessary; just use Promise.resolve
/PromiseResolve
.)
When @rbuckton gets back he should probably be in this discussion.
@domenic Unfortunately getting the global
object is not so easy as just saying "global", as some host environments expose the global object in different ways (e.g. window
in the Browser, self
in a WebWorker, etc.).
@yortus I don't think we will go to exhaustive lengths to resolve the native "Promise". We would have a similar issue for "Math" when using the transpiled **
operator.
Did you get any kind of error when you redefined Promise
? We should be reporting an error if the Promise
we resolve at design-time is not a PromiseConstructor
.
@yortus This doesn't fail because we do not report semantic errors for JavaScript files. If you rename "original.js" to "original.ts" and run the sample, you get the following error:
error TS2403: Subsequent variable declarations must have the same type. Variable 'Promise' must be of type 'PromiseConstructor', but here has type 'string'.
I do see an issue that we do not report an error if "original.ts" is a module for an async function with an inferred return type. I will look into providing a better error for this.
@rbuckton understandable re global
/window
checking being too hard. However if TSC just checks that Promise
is of type PromiseConstructor
, it's still trivially easy to produce valid ES6/ES7 code that does one thing when transpiled, and something different when run directly. For example
// file: test.ts
import * as Promise from 'bluebird';
var foo = async () => (await Promise.delay(500), 42);
foo()
.then(value => console.log(`Result: ${value}`))
.catch(error => console.log(`Error: ${error}`))
.finally(() => console.log('All done!'));
// OUTPUT of "node ./test.js" after running "tsc --target es6 --module commonjs"):
// Result: 42
// All done!
// OUTPUT of running test.ts code directly in MS Edge (no tsc step):
// SCRIPT438: Object doesn't support property or method 'finally'
It's not hard to see this happening in real code. People are starting to use ES7 async functions. And things like bluebird
are still actively developed and have many useful additional features (like Promise.delay
and Promise#finally
used above). By habit or intention bluebird
will still be imported into to module-level variable Promise
. This is valid and won't have any impact on ES7 async function semantics.
Yet in this situation TSC produces code with different semantics, currently without any compiler warnings. The code may behave differently in unexpected ways depending how/whether it's transpiled. And we are talking about a situation with well-defined ES semantics (unless there are significant changes to https://tc39.github.io/ecmascript-asyncawait/ between stage 3 and 4).
The upshot of my previous comment is that the code in test.ts
(above) has unambiguous ES6/ES7 semantics** and should fail, whether transpiled or not. And TSC should know at compile time that it will fail, because it statically knows async functions return ES6 Promises which don't have a finally
method.
** unless significant changes to https://tc39.github.io/ecmascript-asyncawait/ between stage 3 and 4
We have the same issue if you use --target ES5 --allowJs --module commonjs
with this code:
// original.js
var Object = "What could possibly go wrong?";
export class C {
get X { return 1; }
}
// downlevel.js
var Object = "What could possibly go wrong?";
var C = (function () {
function C() {
}
Object.defineProperty(C.prototype, "X", { // throws
get: function() { return 1; }
enumerable: true,
configurable: true
});
return C;
})();
exports.C = C;
@yortus We cannot always guarantee 100% support for ES7 semantics when transpiling down to ES6/ES5. If you run your test.ts
example above in an ES6 host, you wouldn't get an error. This is because the official spec proposal doesn't use the global Promise object either, but rather uses NewPromiseCapability. Once TypeScript supports --target ES7
, we would use instead use native async functions.
What we can do is do the same thing we do for require
and exports
, and reserve Promise
at the top level of a module that contains any async functions/methods. This means that if you want to use bluebird with async functions, you cannot import bluebird with the name Promise
.
import * as Promise from "bluebird";
export var foo = async () => Promise.delay(500);
Would report the error:
// error TS2529: Duplicate identifier 'Promise'. Compiler reserves name 'Promise' in the top level scope of a module containing async functions.
As a result, you would not be able to create a local binding for Promise
, and would instead need to do something like:
import * as bluebird from "bluebird";
export foo = async () => bluebird.delay(500); // type: Promise<void>
if you want to use bluebird with async functions, you cannot import bluebird with the name
Promise
.
I think that would be a good policy given how downlevelling is done. It will catch at compile time what could otherwise be confusing runtime errors.
we can [...] reserve
Promise
at the top level of a module that contains any async functions/methods
How about the following idea? Instead of reserving Promise
at the top of files containing async functions, what about introducing a new helper like __getGlobalObject
that gets the global object, then using it to reference _global.Promise
when downlevelling async functions. Then the compiler would not need to raise an error for local Promise
imports, which are after all valid code.
A quick google suggests getting the global object is not so hard as it seems, eg:
function __getGlobalObject() {
try {
// Get the global object in ES3/5/6/7, in either strict and non-strict mode
return Function('return this')();
}
catch(e) {
// Must be running in browser with Content Security Policy headers set
return window;
}
}
This helper might in fact be useful for transpiling any file that defines local aliases of global objects that are needed by other helpers, such as Object
and Promise
. All the sample code in this issue would retain runtime fidelity if transpiled this way.
As I understand it, using eval
or Function
wouldn't work in sandboxed environments (i.e. SES). If we needed to get the global object, what we've considered is:
var __global = typeof global !== "undefined" ? global : typeof self !== "undefined" ? self : undefined;
That still has issues with anyone locally defining "global" or "self" or "window", which puts us back in the same boat.
That still has issues with anyone locally defining "global" or "self" or "window", which puts us back in the same boat.
I've seen countless files with a locally defined Promise
variable, but I can't recall ever seeing real-world code that redefines global
or window
.
Taking the __global
approach will allow people to start introducing async functions without touching other code. A lot of this code no doubt is currently using promises.
OTOH reserving Promise
means that people introducing async functions into existing code may have to wade through compiler errors requiring them to change their existing code to remove local Promise
aliases. Which is a little annoying knowing that the code is valid E6/ES7 and the errors are just artifacts of the downlevelling implementation.
Pretty sure Function("this")
works - @kitsonk and I looked into this a bit ago.
Yes, it works as long as you are not running inside a sandboxed host like Caja/Secure ECMAScript (SES).
sandboxed host like Caja/Secure ECMAScript (SES)
If that's a small fraction of the TypeScript user base, which I imagine it is, they could provide their own __getGlobalObject
helper, just like in other special cases where authors can provide their own helpers such as __extends
.
It would be good to favour the common case IMHO. I'd prefer to continue allowing code with import Promise...
to work correctly without modifications. For those projects with special environments, they may need to eg define __getGlobalObject = () => global
in their project, and this can be documented.
EDIT: maybe a helper __getGlobalPromise
would be better to support envs like Caja which has no referencable global object.
Has nobody mentioned the obvious solution that instead of reserving Promise
TypeScript could reserve $$_$_$Promise
or a similar unlikely-to-be-used name?
@domenic I am considering that approach as well as several others. Another direction is to rename locally defined variables or imports named Promise
. The reason we are considering the error is to align with our current behavior of reserving require
and exports
at the top level of a module.
Is caja even alive? I've been trying to mess around with it but both playgrounds seem to have link-rotted. The main playground says 'Updated Dec 22, 2009'.
@DanielRosenwasser actually it was this:
const globalObject: any = Function('return this')();
export default globalObject;
@kitsonk Yup, sorry about that. Thanks for the correction.
@kitsonk @DanielRosenwasser Function('return this')()
will throw an exception in browsers on a page with content securty policy that doesn't explicitly allow unsafe-eval
(more info).
That's why I suggested adding a new helper like __getGlobalObject() { return Function('return this')(); }
that can be overridden in projects that run in special environments.
That's why I suggested adding a new helper like
__getGlobalObject() { return Function('return this')(); }
that can be overridden in projects that run in special environments.
Out of curiosity, what would you replace it with in those environments? Is there any options when running under the strict prolog then?
@kitsonk probably return window
would cover most cases, since CSP is a browser thing. But then web workers don't have window
so would need return self
.
I wrote in an earlier comment one possible default implementation that would cover most cases:
function __getGlobalObject() {
try {
// Get the global object in ES3/5/6/7, in either strict and non-strict mode
return Function('return this')();
}
catch(e) {
// Must be running in browser with Content Security Policy headers set
return window;
}
}
I spoke with @mhegazy and for now we will issue an error per this comment. I should have a PR for this shortly, once a prerequisite PR has been merged.
Reserving Promise
should probably be added to breaking changes if the PR lands in a release.
@rbuckton can you clarify if the PR you mentioned is being considered as a stopgap solution, or more final?
Quick search on github for TypeScript code containing import Promise
yields ~26000 results: https://github.com/search?l=typescript&q=import+Promise&ref=searchresults&type=Code&utf8=%E2%9C%93
Obviously only some fraction of that will introduce async/await and hit this new error, but at least it shows it's fairly widespread practice to use a local Promise
identifier (and valid ES6).
It will cause a lot of breakage in Dojo 2 I suspect, as we shim it without touching the global namespace, because we can't be sure we aren't running with other code that doesn't pollute it. Even if we don't use async/await, anyone wanting to use it will end up likely breaking their code and may not be totally clear on why.
anyone wanting to use it will end up likely breaking their code and may not be totally clear on why
Agreed. The error message they will see is:
Duplicate identifier 'Promise'. Compiler reserves name 'Promise' in the top level scope of a module containing async functions.
When in fact Promise
has no duplicate definition in the module's scope (unlike redefining require
or exports
for example).
Also it may be unclear for experienced JS devs why TS rejects this code, since it always worked before and is valid ES6.
@yortus The message is consistent with the error we report for using require
or exports
at the top level of a module as well. We are considering whether we should update the messaging around these errors across all similar cases as part of a different change at a later date.
This is fixed as of #6631. Please refer to the following comments as to the reasons and rationale for this change:
We will be updating our Breaking Changes documentation for TypeScript 1.8 to inform users as to the change and its implications.
In the following example, running
original.js
directly in MS Edge 25 / EdgeHTML 13 (with experimental JavaScript enabled) outputs"Get to the chopper!"
to the console.downlevel.js
is produced by runningtypescript@next
(I used v1.8.0-dev.20151210) overoriginal.js
with the--allowJs
option. Runningdownlevel.js
in MS Edge outputs an errorSCRIPT445: Object doesn't support this action
.It looks like the down-levelled code picks up the locally-scoped
Promise
instead of using the nativePromise
.