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

Array destructuring is transpiled with __read helper even when asking for no transpilation and no inline helper functions. #43541

Open
blickly opened this issue Apr 6, 2021 · 11 comments
Assignees
Labels
Needs Investigation This issue needs a team member to investigate its status. Rescheduled This issue was previously scheduled to an earlier milestone

Comments

@blickly
Copy link

blickly commented Apr 6, 2021

Bug Report

πŸ”Ž Search Terms

array destructuring
downLevelIteration
importHelpers
ESNEXT
CommonJS

πŸ•— Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about importHelpers

⏯ Playground Link

Playground link with relevant code

Note: I'm not sure how to reliably get the target option to encode into the URL; you may need to set the target manually to ESNext (or any other value >= ES2015)

πŸ’» Code

declare function foo(): any;

export const [A, V] = foo();

πŸ™ Actual behavior

The __read transpilation helper was injected into the output JS, even though I specified that I didn't want transpilation (through target=ESNEXT). Additionally, the transpilation helper was injected inline even though I specified the importHelpers=true option.

"use strict";
var __read = (this && this.__read) || function (o, n) {
    var m = typeof Symbol === "function" && o[Symbol.iterator];
    if (!m) return o;
    var i = m.call(o), r, ar = [], e;
    try {
        while ((n === void 0 || n-- > 0) && !(r = i.next()).done) ar.push(r.value);
    }
    catch (error) { e = { error: error }; }
    finally {
        try {
            if (r && !r.done && (m = i["return"])) m.call(i);
        }
        finally { if (e) throw e.error; }
    }
    return ar;
};
var _a;
Object.defineProperty(exports, "__esModule", { value: true });
exports.V = exports.A = void 0;
_a = __read(foo(), 2), exports.A = _a[0], exports.V = _a[1];

πŸ™‚ Expected behavior

I was expecting output that leveraged the target=ESNEXT setting to include the array destructuring in the output code, like:

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
exports.V = exports.A = void 0;
const [A, V] = foo();
exports.A = A;
exports.V = V;
@blickly blickly changed the title Array destructuring Array destructuring is transpiled with __read helper even when asking for no transpilation and no inline helper functions. Apr 6, 2021
@andrewbranch
Copy link
Member

The __read helper was used even though the target was ESNext due to --downlevelIteration being enabled. However, it does seem like --importHelpers should have imported it instead of inlining it. Interestingly, if another helper is used, __read gets imported along with it: https://www.typescriptlang.org/play?downlevelIteration=true&importHelpers=true&target=7&module=1#code/CYUwxgNghgTiAEAzArgOzAFwJYHtVJxwAoBKALnilQE8BuAKBAA8AHHGDeMPAZ04G0AggBp4ANQC68ALwFiJBvSwBbNh3gAqSj3gAjaAAskMHMvgByRIXO14AejtdTykKk45knDAaw6IWVBB6fSgDBiA

@rbuckton, is this intentional, or is __read just forgetting to signal that the helpers need to be imported?

@andrewbranch andrewbranch added the Needs Investigation This issue needs a team member to investigate its status. label Apr 6, 2021
@andrewbranch andrewbranch added this to the TypeScript 4.3.1 milestone Apr 6, 2021
@blickly
Copy link
Author

blickly commented Apr 6, 2021

I'm happy to change my flags if I can both avoid the helpers and get correct behavior. Unfortunately, when I turn off --downlevelIteration, I get incorrect behavior that doesn't work with iterables:

"use strict";
var _a;
Object.defineProperty(exports, "__esModule", { value: true });
exports.V = exports.A = void 0;
_a = foo(), exports.A = _a[0], exports.V = _a[1];

@rbuckton
Copy link
Member

rbuckton commented Apr 7, 2021

We use the __read helper to downlevel the export, even though you're using --target esnext, because you've specified --module commonjs. This should be importing it from tslib if --importHelpers is used, since the file is a module.

We could also investigate updating the module.ts transformer to rewrite the output as follows, when using --target ES2015 or higher:

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
exports.V = exports.A = void 0;
[exports.A, exports.V] = foo();

I think we do the general downleveling for the sake of emit simplicity, rather than --target consistency.

@ilogico
Copy link

ilogico commented Apr 7, 2021

Here's an interesting variation:

declare function foo(): any;
const [A, V] = foo();
export { A, V }

Changing the export syntax prevents the downleveling.

@blickly
Copy link
Author

blickly commented Apr 8, 2021

@rbuckton, I agree that in the long term, we should eventually switch from --module commonjs to --module esnext, at which point I assume that the --downlevelIteration will become a no-op since the tslib helper methods will all be unneeded (I hope).

For the time being, is there any guidance on which code patterns will need to depend on the tslib helpers in the output?

@RyanCavanaugh RyanCavanaugh added the Rescheduled This issue was previously scheduled to an earlier milestone label Jun 18, 2021
@wehrstedt
Copy link

wehrstedt commented Jul 16, 2021

I have the same problem with a for-of loop, but I think I found the trick:

script1.ts:

const array = [1, 2, 3];
for (const value of array) {
	console.log(value)
}

will be transpiled to

var __values = (this && this.__values) || function(o) {
    var s = typeof Symbol === "function" && Symbol.iterator, m = s && o[s], i = 0;
    if (m) return m.call(o);
    if (o && typeof o.length === "number") return {
        next: function () {
            if (o && i >= o.length) o = void 0;
            return { value: o && o[i++], done: !o };
        }
    };
    throw new TypeError(s ? "Object is not iterable." : "Symbol.iterator is not defined.");
};
var e_1, _a;
var array = [1, 2, 3];
try {
    for (var array_1 = __values(array), array_1_1 = array_1.next(); !array_1_1.done; array_1_1 = array_1.next()) {
        var value = array_1_1.value;
        console.log(value);
    }
}
catch (e_1_1) { e_1 = { error: e_1_1 }; }
finally {
    try {
        if (array_1_1 && !array_1_1.done && (_a = array_1.return)) _a.call(array_1);
    }
    finally { if (e_1) throw e_1.error; }
}
//# sourceMappingURL=test2.js.map

script2.ts:

export function fuu() {
	const array = [1, 2, 3];
	for (const value of array) {
		console.log(value)
	}
}

fuu();

will be correctly transpiled to

Object.defineProperty(exports, "__esModule", { value: true });
exports.fuu = void 0;
var tslib_1 = require("tslib");
function fuu() {
    var e_1, _a;
    var array = [1, 2, 3];
    try {
        for (var array_1 = tslib_1.__values(array), array_1_1 = array_1.next(); !array_1_1.done; array_1_1 = array_1.next()) {
            var value = array_1_1.value;
            console.log(value);
        }
    }
    catch (e_1_1) { e_1 = { error: e_1_1 }; }
    finally {
        try {
            if (array_1_1 && !array_1_1.done && (_a = array_1.return)) _a.call(array_1);
        }
        finally { if (e_1) throw e_1.error; }
    }
}
exports.fuu = fuu;
fuu();
//# sourceMappingURL=test1.js.map

so the difference is that in the second script, which imports tslib (this is what is expected), the code is wrapped by a function. In the first script the code is in the global scope and then typescript emits the helper functions as expected.

@wehrstedt
Copy link

If you remove exort from script2 it breaks. so the key seems to be the export keyword?

@frigus02
Copy link
Contributor

Hi there. I started looking into this. Here is my current understanding of why this happens. This is roughly the order of transformations in the compilation process:

  1. Transform language features, such as async/await, rest/spread, etc. TS collects required helpers throughout the process.
  2. Take the collected helpers and decide how to integrate them (import or inline). code
  3. Transform code to the specified module system, e.g. CommonJS (code). In this step TS encounters the destructuring assignment (code) and wants to apply the downlevel iteration transformation for which it requires another helper (code). However also at this point the decision about import or inline has already been made. And in case the transforms in step 1 didn't need any helpers, the default is inline. So this additional helper ends up being inlined.

This means that importHelpers is generally respected. The only case where it's ignored is when the helper is required in an import/export statement and it's the only required helper in the file.

I see 2 ways to fix this:

  • Make the decision about import or inline helpers after the module transformation. I think the tricky bit might be that a potential helper import should also run through the module transformation itself.
  • Ensure that the module transformation does not require any additional helpers. It sounds this currently only happens to the sake of simplicity.

Is my understanding correct? And which option would you prefer?

I can try to work on a PR. But would like to make sure I go in the right direction πŸ™‚

@blickly
Copy link
Author

blickly commented Aug 25, 2021

From a user's perspective, I think it would be nicer if the module transformation didn't depend on helpers that are not used in the --target level that was requested.

But I think folks on the MS side might have a preference on which approach would fit better with the TSC architecture.

@rbuckton rbuckton removed the Working as Intended The behavior described is the intended behavior; this is not a bug label Mar 15, 2024
@rbuckton
Copy link
Member

Specifying --target ESNext does not necessarily mean there is no transpilation. What you are seeing is a side-effect of using --target commonjs and export const [A, V] = foo(). This needs to be transpiled to assignments to exports.A and exports.V. While we could possibly transpile this to [exports.A, exports.V] = foo(), the module transformer happens at the end of all of the other transformation passes and needs to work regardless as to the --target you specify. Rather than duplicate the work we do in the earlier language-specific transformers, we opted to instead perform a general destructuring transformation that is --target agnostic.

Any change here would require branching on the target ECMAScript edition to control how much of the destructuring transform we actually need.

@rbuckton
Copy link
Member

Aside from this, there's no other reason to use --downlevelIteration other than for --target ES5. I'd considered closing this as "Working as Intended", but I'd rather make it an error to use --downlevelIteration with the other script targets. To do that means addressing the few cases where it matters in our module transformers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Investigation This issue needs a team member to investigate its status. Rescheduled This issue was previously scheduled to an earlier milestone
Projects
None yet
Development

No branches or pull requests

7 participants