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

Recursive Node.js require #13245

Closed
calebmer opened this issue Jan 1, 2017 · 10 comments
Closed

Recursive Node.js require #13245

calebmer opened this issue Jan 1, 2017 · 10 comments
Labels
Bug A bug in TypeScript
Milestone

Comments

@calebmer
Copy link

calebmer commented Jan 1, 2017

Recursive modules with exporting files in-between don’t work with TypeScript because export { ... } from '...' statements extract the exported property when the file first runs instead of using a getter to lazily extract the exported property as Babel does.

When Node.js detects recursive modules, they emit the module exports that have been initialized thus far. See module cycles documentation. This means an export may be undefined in the required module when a cycle is detected. However, that export will later be defined when both modules finish their initial execution. Because TypeScript does not use getters there is no way of getting the export when it is later defined.

Here is a concrete example to demonstrate the recursive problem in Node.js. In the bug template is the output expected for a given file. The below example would create an infinite recursive loop when a or b is called. This is the desired behavior. What actually happens is that either b or a is undefined and can’t be called.

./a/foo.ts

import { b } from '../b'

export function a () {
  b()
  return 'foo'
}

./a/index.ts

export { a } from './foo'

./b/bar.ts

import { a } from '../a'

export function b () {
  a()
  return 'bar'
}

./b/index.ts

export { b } from './bar'

Expected/Actual

TypeScript Version: 2.1.4

Code

export { a } from './foo'
export { b } from './bar'

Expected behavior:

Emits getters for CommonJS exports like Babel:

'use strict';

Object.defineProperty(exports, "__esModule", {
  value: true
});

var _foo = require('./foo');

Object.defineProperty(exports, 'a', {
  enumerable: true,
  get: function get() {
    return _foo.a;
  }
});

var _bar = require('./bar');

Object.defineProperty(exports, 'b', {
  enumerable: true,
  get: function get() {
    return _bar.b;
  }
});

Actual behavior:

Extracts the imported values on file execution so if the reference changes in the required module, that change does not propagate:

"use strict";
var foo_1 = require("./foo");
exports.a = foo_1.a;
var bar_1 = require("./bar");
exports.b = bar_1.b;

Tasks

No tasks being tracked yet.
@aluanhaddad
Copy link
Contributor

I think this is a duplicate of #12522.

@calebmer
Copy link
Author

calebmer commented Jan 2, 2017

The cause is the same as in #12522, but the effect is different. In this case it breaks Node.js support for cyclical dependencies.

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Jan 2, 2017

Technically that's not node support but Babel support right? My understanding is that this behavior is enabled by a translating interpretation of ES Modules onto CommonJS. (I could be very wrong about this)

@calebmer
Copy link
Author

calebmer commented Jan 3, 2017

I am pretty sure that es modules allow for recursive imports. My example above I believe to be valid ECMAScript so it’s not so much an issue of translating interpretation as it is of spec compliance. Maybe I misunderstood?

@aluanhaddad
Copy link
Contributor

The reason I bring it up is that Node does not support ES Modules so all targeting code has to be transpiled and is subject to interpretation as a result. When you use --module commonjs, tsc outputs code that targets CommonJS Modules directly and this is a very different thing than targeting ES Modules. That said, perhaps the current behavior also stems from the inherent support that TypeScript has for CommonJS/Node ala the specialized

import a = require('./a');

syntax that is unique to TypeScript and naturally translates to

var a = require('./a');

Arguably

import {a} from 'a';

should translate to emit that maps the ES specified behavior to Node constructs.
That would also possibly mean revisiting the emit behavior of

import a from './a';

to reference a synthetic default import.

Basically, Babel is treating CommonJS Modules as a legacy target for ES Modules while TypeScript is treating CommonJS as a first class format yet it is supporting ES Module syntax alongside and in terms of the require function. The more I think about it, the more this seems inherently problematic...

@calebmer
Copy link
Author

calebmer commented Jan 9, 2017

Ok but inherent problems aside I don’t see anything that really blocks changing the emit behavior from this:

"use strict";
var foo_1 = require("./foo");
exports.a = foo_1.a;
var bar_1 = require("./bar");
exports.b = bar_1.b;

To this:

'use strict';

var foo_1 = require('./foo');
Object.defineProperty(exports, 'a', {
  enumerable: true,
  get: () => foo_1.a,
});

var bar_1 = require('./bar');
Object.defineProperty(exports, 'b', {
  enumerable: true,
  get: () => bar_1.b,
});

This results in the exact same behavior that is being emit now. Technically you can’t configure or set to these properties (I’m not sure why anyone would), but if you wanted strict backwards compatibility you just change the emit to this:

var foo_1 = require('./foo');
Object.defineProperty(exports, 'a', {
  configurable: true,
  enumerable: true,
  get: () => foo_1.a,
  set: value => { foo_1.a = value; },
});

Then we really do have the exact same behavior.

So this would not change the TypeScript interpretation of the ES Module and CommonJS module systems. After all it behaves the same. The only thing this changes is it fixes an inconsistency in how TypeScript handles the ES Module spec specifically in the case of recursive Node.js requires.

If you worry about adding bytes to your application code, then the state of the art for JavaScript bundling is to not transpile to CommonJS and instead let your bundler (Webpack or Rollup most likely) to intelligently handle your ES Module declarations (and it’s worth noting that they handle this case as well).

I guess from my perspective I’m not seeing any inherent problems. This seems like a net positive to me that does not effect any existing code and fixes a specific bug that I (and likely Babel users too given Babel’s behavior) have been experiencing 😖

@tristan957
Copy link

This issue is extremely annoying in applications using NestJS (dependency injection). Referencing enums and decorators outside the modules in which they are defined through reexports impossible. The TypeScript compiler complains about them not existing when they are. The behavior is easily reproducible and in my opinion is a bug.

@squidfunk
Copy link

I started a PR in #31715 using the technique described by @calebmer 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
@RyanCavanaugh
Copy link
Member

Possibly same root cause as #12522

@jakebailey
Copy link
Member

Old issue, but this was fixed back in TS 3.9; the example code from the original post is now:

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
exports.b = exports.a = void 0;
var foo_1 = require("./foo");
Object.defineProperty(exports, "a", { enumerable: true, get: function () { return foo_1.a; } });
var bar_1 = require("./bar");
Object.defineProperty(exports, "b", { enumerable: true, get: function () { return bar_1.b; } });

So, it is now using a getter like babel did.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript
Projects
None yet
Development

No branches or pull requests

6 participants