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

[Bug] Re-exported binding doesn't track the original binding #12522

Closed
yortus opened this issue Nov 27, 2016 · 11 comments · Fixed by #35967
Closed

[Bug] Re-exported binding doesn't track the original binding #12522

yortus opened this issue Nov 27, 2016 · 11 comments · Fixed by #35967
Labels
Breaking Change Would introduce errors in existing code Bug A bug in TypeScript

Comments

@yortus
Copy link
Contributor

yortus commented Nov 27, 2016

TypeScript Version: nightly (2.2.0-dev.20161127)

EDIT: The issue described below is with --module commonjs, but as @aluanhaddad points out in a comment below, there are other behaviours with different module emits.

I looked for an existing issue about this but didn't find one. Apologies if this is a duplicate.

I ran into a problem with TypeScript's emit for re-exported bindings, in particular when the original exported value changes over time.

Importing a binding directly from a source module gets a 'live view' that changes when the export value changes, as it should. But importing from a re-exported binding gets a snapshot that is no longer a live view of the original export. This seems not to be spec compliant and leads to my code not working as intended.

Code

// file: a.ts
export let liveView = 'foo';
export function changeTo(val: string) { liveView = val; }   // CHANGES THE EXPORT VALUE  



// file: b.ts
import {liveView, changeTo} from './a';     // <===== Import directly from A
console.log(liveView); // outputs 'foo'
changeTo('bar');
console.log(liveView); // outputs 'bar'        <===== GOOD: binding changed as it should

export {liveView, changeTo} from './a';     // <===== Re-export from A



// file: c.ts
import {liveView, changeTo} from './b';     // <===== Import via B's re-export
console.log(liveView); // outputs 'bar'
changeTo('baz');
console.log(liveView); // outputs 'bar'     // <===== BAD: binding is not live anymore

Expected behavior

Emitted code should preserve the live binding to liveView when re-exporting from b.ts.

Last line of c.ts should output 'baz'.

Actual behavior

Emitted code exports a 'snapshot' of the liveView binding from b.ts, so that the import of liveView into c.ts is no longer a live binding.

Last line of c.ts incorrectly outputs 'bar'.

What should be the Correct Behaviour?

I found it hard to locate a clear description of correct ES6 behaviour for this situation, but from what I did find I think TypeScript's current emit is not compliant with ES6.

So for export { liveView } from 'some-module'; TypeScript emits something like:

var some_module_1 = require("some-module");
exports.liveView = some_module_1.liveView;

Whereas babel emits something like:

var _someModule = require('some-module');

Object.defineProperty(exports, 'liveView', {
  enumerable: true,
  get: function get() {
    return _someModule.liveView;
  }
});
@yortus yortus changed the title Emit for re-exports loses live binding (not ES6 compliant) [Bug] Re-exported binding doesn't track the original binding Nov 29, 2016
@aluanhaddad
Copy link
Contributor

This is really interesting. Note that is works differently in different module formats. Specifically if you compile into system modules, the output is actually

foo
bar
bar
baz

as you expect it to be because the vars are declared outside of the closure which sets up the module and are hoisted. Also, using any module format except system results in output which imports a twice in b. Another curious aspect is how reordering the statements changes the behavior.
If we alter b by moving the re-export above the call to changeTo
and transpile to commonjs, the output changes to

foo
bar
foo
foo

the behavior under the system format does not change.

@yortus
Copy link
Contributor Author

yortus commented Dec 4, 2016

That is interesting. I should have mentioned in the OP that I was using commonjs. Added it now.

@yortus
Copy link
Contributor Author

yortus commented Dec 8, 2016

This seems to have slipped through the cracks. @mhegazy would you mind triaging this issue?

@aluanhaddad
Copy link
Contributor

The current state of the node-eps for es modules makes this very pertinent. It definitely needs to be revisited.

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label May 24, 2017
@weswigham weswigham added the Bug A bug in TypeScript label Feb 27, 2018
@mhegazy mhegazy removed the Bug A bug in TypeScript label Apr 12, 2018
@squidfunk
Copy link

squidfunk commented Jan 22, 2019

Any update on this? Still happening and wildly indeterministic. Also, when I add a simple console.log statement or even an empty comment to the file that re-exports everything inside that folder, it magically works:

Doesn't work (foo is not re-exported):

export * from "./foo"
export * from "./bar"

Works:

//
export * from "./foo"
export * from "./bar"

Also works:

console.log("")
export * from "./foo"
export * from "./bar"

I'm also using commonjs.

EDIT: Following this comment – it seems to vanish when removing "strict": true from tsconfig.json. I added all flags that are enabled by "strict": true manually.

@ghost
Copy link

ghost commented Mar 16, 2019

Is this issue still being worked on? This can be a serious problem in some cases (such as this), and is definitely out of the ESM spec.

Also, the tricks mentioned by @squidfunk are invalid currently.

@squidfunk
Copy link

squidfunk commented Mar 16, 2019

@t532 - removing strict: true still works for me with TypeScript (3.3.3333)

@squidfunk
Copy link

Unfortunately @t532 is right - I've just encountered a case where setting strict: true doesn't work. Needs investigation.

@squidfunk
Copy link

What's the state of this issue? Is it considered being investigated?

The issue was triaged as "needs investigation" by @RyanCavanaugh nearly two years ago, and shortly after that labeled as a "bug", but a year ago it was not considered being a bug anymore? Its effects are wildly indeterministic and thus really annoying. We're in need of deterministic builds and re-exports are a very common thing. This problem may live in a lot of codebases.

I'm currently working around it by adding an empty comment after the export statement of the dependency that is not correctly being re-exported, but that's far from a solution.

@ghost
Copy link

ghost commented May 3, 2019

@squidfunk Well I don't think this is being actually investigated anymore - the solution is clear: make the imported values getters, just as what Babel and Rollup did; but for some reason they are not getting this done. @weswigham Would you mind paying some attention to this?

A little complaint: I filed the same issue in Rollup and it got solved in 2 weeks. However this issue is still open after 2 years of "investigation".

@squidfunk
Copy link

I started a PR in #31715 using the technique described in #13245 (comment). Heavily WIP. Happy to collaborate!

@RyanCavanaugh RyanCavanaugh added Bug A bug in TypeScript and removed Needs Investigation This issue needs a team member to investigate its status. labels Aug 22, 2019
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Aug 22, 2019
@DanielRosenwasser DanielRosenwasser added the Breaking Change Would introduce errors in existing code label May 12, 2020
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 Bug A bug in TypeScript
Projects
None yet
7 participants