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

Can __export emitter be controlled by emitEmitHelpers compile option? #10905

Closed
meenie opened this issue Sep 13, 2016 · 9 comments
Closed

Can __export emitter be controlled by emitEmitHelpers compile option? #10905

meenie opened this issue Sep 13, 2016 · 9 comments
Labels
Breaking Change Would introduce errors in existing code Help Wanted You can do this Suggestion An idea for TypeScript
Milestone

Comments

@meenie
Copy link

meenie commented Sep 13, 2016

I'm trying to achieve 100% code coverage and the function:

function __export(m) {
    for (var p in m) if (!exports.hasOwnProperty(p)) exports[p] = m[p];
}

is added in every file that does something like export * from './blah'.

Can that code be controlled by the compilerOptions.emitEmitHelpers flag? Then it can be added to https://github.com/ngParty/ts-helpers and code coverage can go to 100% :).

I'm using https://github.com/hapijs/lab and it reports this:
image

@meenie
Copy link
Author

meenie commented Sep 15, 2016

This in the "too hard" bucket? :)

@kitsonk
Copy link
Contributor

kitsonk commented Sep 15, 2016

Can that code be controlled by the compilerOptions.emitEmitHelpers flag?

Yes.

Option Type Default Description
--noEmitHelpers boolean false Do not generate custom helper functions like __extends in compiled output.

@meenie
Copy link
Author

meenie commented Sep 15, 2016

@kitsonk Right... All the other helpers are controlled by that option, except for __export().

@RyanCavanaugh
Copy link
Member

@rbuckton what's the intended behavior of the flag vs __export ?

@mhegazy
Copy link
Contributor

mhegazy commented Sep 16, 2016

We could elevate it to be a real, i.e. check if there is an override, do not emit it with --noEmitHelpers and import it from a helper library with --importHelpers (TS 2.1 feature).

one thing to note this will be a breaking change for users with --noEmitHelpers today, but i would say it an't too bad.

A PR would be appreciated. you will need to change the helper itself to not be based on closure, emit it at the top along with other helpers, and make sure it is only emitted when needed, and add a check to the make sure that external helper libraries implement it (using verifyHelperSymbol).

@mhegazy mhegazy added Suggestion An idea for TypeScript Help Wanted You can do this Breaking Change Would introduce errors in existing code labels Sep 16, 2016
@rbuckton
Copy link
Member

The __export helper closes over the exports value, and is unique to every module, which is why it does not respect this switch.

We could consider adding an __export helper using the following definition instead:

var __export = (this && this.__export) || function (exports, m) {
  for (var p in m) if (!exports.hasOwnProperty(p)) exports[p] = m[p];
}

And change all call sites to pass in the exports declaration. However, this would be a breaking change for anyone who currently uses --noEmitHelpers that currently depends on __export.

@meenie Another option is to add a custom lab transform for the __export function, per the documentation:

  • -T , --transform - javascript file that exports an array of objects ie. [ { ext: ".js", transform: (content, filename) => { ... } } ]. Note that if you use this option with -c (--coverage), then you must generate sourcemaps and pass sourcemaps option to get proper line numbers.

@meenie
Copy link
Author

meenie commented Sep 16, 2016

Another option is to add a custom lab transform for the __export function, per the documentation:

@rbuckton That's what I ended up doing :). This should still be addressed, though. I assume it's affecting other people.

@gilboa23
Copy link

gilboa23 commented Dec 31, 2016

@rbuckton Yes, but I was not referring to the existing --noEmitHelpers flag. What I meant was to apply this change just over the new --importHelpers flag by passing the local exports symbol in the invocations of the imported tslib.__export() function. This should not be a breaking change since this is a now starting to be used. Something like:

// @module: amd
// @importHelpers: true
export * from "A";
export * from "B";

Output:

define(["require", "exports", "tslib", "A", "B"], function (require, exports, tslib_1, A_1, B_1) {
    "use strict";
    tslib_1.__export(A_1, exports);
    tslib_1.__export(B_1, exports);
});

See also related comments in PR 9097

@mhegazy mhegazy added this to the Community milestone Jan 4, 2018
@RyanCavanaugh RyanCavanaugh modified the milestones: Community, Backlog Mar 7, 2019
@RyanCavanaugh
Copy link
Member

It doesn't seem like anyone has really needed this, and AMD emit is quite rare anymore.

@RyanCavanaugh RyanCavanaugh closed this as not planned Won't fix, can't repro, duplicate, stale Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Would introduce errors in existing code Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

6 participants