module: warn on using unfinished circular dependency by addaleax · Pull Request #29935 · nodejs/node · GitHub

Warn when a non-existent property of an unfinished module.exports
object is being accessed, as that very often indicates the presence
of a hard-to-detect and hard-to-debug problem.

This mechanism is only used if module.exports is still a
regular object at the point at which the second, circular require()
happens.

The downside is that, temporarily, module.exports will have a
prototype other than Object.prototype, and that there may
be valid uses of accessing non-existent properties of unfinished
module.exports objects.

Performance of circular require calls in general is not
noticeably impacted.

                                           confidence improvement accuracy (*)   (**)  (***)
 module/module-loader-circular.js n=10000                 3.96 %       ±5.12% ±6.82% ±8.89%

Example:

$ cat a.js 
'use strict';
const b = require('./b.js');

exports.fn = () => {};
$ cat b.js 
'use strict';
const a = require('./a.js');

a.fn();
$ node a.js 
(node:1617) Warning: Accessing non-existent property 'fn' of module exports inside circular dependency
/tmp/b.js:4
a.fn();
  ^

TypeError: a.fn is not a function
    at Object.<anonymous> (/tmp/b.js:4:3)
    [...]

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Warn when a non-existent property of an unfinished module.exports object is being accessed, as that very often indicates the presence of a hard-to-detect and hard-to-debug problem. This mechanism is only used if `module.exports` is still a regular object at the point at which the second, circular `require()` happens. The downside is that, temporarily, `module.exports` will have a prototype other than `Object.prototype`, and that there may be valid uses of accessing non-existent properties of unfinished `module.exports` objects. Performance of circular require calls in general is not noticeably impacted. confidence improvement accuracy (*) (**) (***) module/module-loader-circular.js n=10000 3.96 % ±5.12% ±6.82% ±8.89% Example: $ cat a.js 'use strict'; const b = require('./b.js'); exports.fn = () => {}; $ cat b.js 'use strict'; const a = require('./a.js'); a.fn(); $ node a.js (node:1617) Warning: Accessing non-existent property 'fn' of module exports inside circular dependency /tmp/b.js:4 a.fn(); ^ TypeError: a.fn is not a function at Object.<anonymous> (/tmp/b.js:4:3) [...]

maybe we could start with warning if module.loaded is false, and look into more advanced messages in future changes?

@devsnek This does only warn if module.loaded is false, so I’m not quite sure what you’re saying?

If you want to warn every time that there’s a cyclic dependency, that would be a non-starter to me because I assume it’s going to be very, very annoying to have a ton of warnings about something that’s perfectly fine if done right.

@addaleax yeah i mean starting with just detecting all circulars. I think most of it is probably working by chance, not by intention. (I'm not blocking anything here though)

@devsnek So … maybe this is just my experience, but I think circular dependencies aren’t all that uncommon, and if they are working, it’s often not just by chance but because somebody figured out that they didn’t work on the first try and then spent a while fixing them. (I definitely had that experience in the past, and iirc the same thing happened with circular dependencies in Node’s internal modules – e.g. 413fcad or 355523f)

So if we do want to warn on all circular modules, I would not make that the default mode, and I would like to have something that warns about potential issues that is on by default.

CI: https://ci.nodejs.org/job/node-test-pull-request/25968/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2050/ (scheduled)

would this somehow impact code that clears the contents the require cache?

I don’t really have time rn to think of and/or show examples of how to break this, but I am a bit concerned about unintended consequences of changing the prototype, and somewhat less concerned about false positives. mainly around modules like proxyquire, rewire, esm, etc

I suspect that these warnings may be more common than we expect in running code adding to noise on existing apps, but apart from that this approach seems fine to me.

@addaleax have you tried running node with this change on an app, or using some CLI tools?

If there was a node --throw-warnings option, we could run CITGM with it... How about we try with an alternative branch that throws to test the impact on the ecosystem?

would this somehow impact code that clears the contents the require cache?

@boneskull I don't think so. The only interaction with the require cache that I can see is what would happen if a module deleted itself from the require cache while it is being loaded (whyever one would do that), but even in that scenario all that this PR would change is that it would not print a warning.

I don’t really have time rn to think of and/or show examples of how to break this, but I am a bit concerned about unintended consequences of changing the prototype, and somewhat less concerned about false positives. mainly around modules like proxyquire, rewire, esm, etc

I can understand that, and I was a bit worried about this when writing the code, but after thinking it through I'm not really seeing this as a major concern. The prototype is always reset after the module is loaded, and only if it hasn't been changed in the meantime anyway, and all methods on it are still there during the whole process. The main way this is observable, besides the warnings, would be when somebody checks whether module.exports.__proto__ === Object.prototype explicitly, but then ... why would one do that?

I suspect that these warnings may be more common than we expect in running code adding to noise on existing apps, but apart from that this approach seems fine to me.

@guybedford Yes, that would be my biggest concern here too. I've run CITGM with that in mind, and I also like @targos' suggestion of turning this a throw and then running CITGM with that.

@addaleax have you tried running node with this change on an app, or using some CLI tools?

Not yet, no.

Actually, it looks like this interferes with TypeScript enums, where something like

export const enum Foo { A = 1, B = 2 };

can result in something like

(function (Foo) {
    Foo[Foo["A"] = 1] = "A";
    Foo[Foo["B"] = 2] = "B";
})(Foo = exports.Foo || (exports.Foo = {}));

so that might actually require getting a bit fancier here… maybe we need to exclude the module file itself from these warnings.

Thinking about it, I think it’s a good idea to exclude transpiled TS/ESM code altogether, at least for now. ESM kind of has a different idea of how to deal with cyclic dependencies anyway, and already throws an explicit error for TDZ accesses.

Here’s a CITGM for this code + throwing instead of warning so that we get meaningful results: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2051/

`lib/pack.js` and `lib/config/figgy-config.js` load each other, making `figgy-config.js` grab the original `module.exports` value and not the intended one. In particular, this always sets the `dirPacker` value to `undefined` in the config generation step. Fix this by setting `module.exports` early. Refs: nodejs/node#29935

So … the bad news is, CITGM is failing because npm pack always encounters this condition.

The good news is: it’s not a false positive, but rather a genuine bug with npm of the exact kind that this PR is supposed to catch. Yay? :)

Here’s another CITGM with a hack to silence the warning for that instance in npm: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2052/

I would gladly welcome a warning like this. People can easily pass the --no-warnings to silence it.

Here's an issue that I'm not sure how to resolve, though. Say someone is already using --no-warnings to silence all the warnings from --experimental-modules et al. Would it then be impossible to opt-in to this particular warning?

Here's an issue that I'm not sure how to resolve, though. Say someone is already using --no-warnings to silence all the warnings from --experimental-modules et al. Would it then be impossible to opt-in to this particular warning?

@DerekNonGeneric So … yes, for the moment that would be the case – --no-warnings does what it says on the tin, it disables all warnings.

I think that brings up an interesting point though, namely whether we should have a way to disable specific kinds of warnings… maybe that would be helpful?

Sifting through the CITGM results:

bluebird: shows genuine bug in outdated version of sinon dependency
q: shows genuine bug in outdated version of sinon dependency
sax: non-bug in shelljs dependency
yeoman-generator: non-bug in shelljs dependency

The other failures seem like unrelated, pre-existing CITGM failures.

I will open a PR against shelljs that addresses this as far as their side is concerned, and suggest maybe landing this as a semver-major PR to be on the safe side and give us time to look into other false positives.

@nodejs/tsc Thoughts on that approach?

Node.js is currently considering printing a warning when a non-existent property of `module.exports` is accessed while in a circular `require()` dependency, in order to make it easier to catch issues with circular dependencies. In order to avoid printing these warnings for shelljs, checking for the property’s existence rather than its truthiness suffices. Refs: nodejs/node#29935

This would definitely be nice to have, at the very least as an opt-in feature.

Node.js is currently considering printing a warning when a non-existent property of `module.exports` is accessed while in a circular `require()` dependency, in order to make it easier to catch issues with circular dependencies. In order to avoid printing these warnings for shelljs, checking for the property’s existence rather than its truthiness suffices. Refs: nodejs/node#29935

CI: https://ci.nodejs.org/job/node-test-pull-request/26168/

Can I get another @nodejs/tsc review? I’ve labelled this semver-major, and if we do run into trouble in the next 6 months, we can still put this behind a flag if necessary.

`lib/pack.js` and `lib/config/figgy-config.js` load each other, making `figgy-config.js` grab the original `module.exports` value and not the intended one. In particular, this always sets the `dirPacker` value to `undefined` in the config generation step. Fix this by setting `module.exports` early. Refs: nodejs/node#29935 PR-URL: #266 Credit: @addaleax Close: #266 Reviewed-by: @mikemimik

CI: https://ci.nodejs.org/job/node-test-pull-request/26246/

Warn when a non-existent property of an unfinished module.exports object is being accessed, as that very often indicates the presence of a hard-to-detect and hard-to-debug problem. This mechanism is only used if `module.exports` is still a regular object at the point at which the second, circular `require()` happens. The downside is that, temporarily, `module.exports` will have a prototype other than `Object.prototype`, and that there may be valid uses of accessing non-existent properties of unfinished `module.exports` objects. Performance of circular require calls in general is not noticeably impacted. confidence improvement accuracy (*) (**) (***) module/module-loader-circular.js n=10000 3.96 % ±5.12% ±6.82% ±8.89% Example: $ cat a.js 'use strict'; const b = require('./b.js'); exports.fn = () => {}; $ cat b.js 'use strict'; const a = require('./a.js'); a.fn(); $ node a.js (node:1617) Warning: Accessing non-existent property 'fn' of module exports inside circular dependency /tmp/b.js:4 a.fn(); ^ TypeError: a.fn is not a function at Object.<anonymous> (/tmp/b.js:4:3) [...] PR-URL: #29935 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Landed in d7452b7

Since this property access is performed by generated code, and not used for accessing the actual exports of a module (and because transpilers generally define it as the first key of `module.exports` when it *is* present), it should be okay to allow it. Refs: nodejs#29935 Fixes: nodejs#33046

Node.js is currently considering printing a warning when a non-existent property of `module.exports` is accessed while in a circular `require()` dependency, in order to make it easier to catch issues with circular dependencies. In order to avoid printing these warnings for shelljs, checking for the property’s existence rather than its truthiness suffices. Refs: nodejs/node#29935

Since this property access is performed by generated code, and not used for accessing the actual exports of a module (and because transpilers generally define it as the first key of `module.exports` when it *is* present), it should be okay to allow it. Refs: #29935 Fixes: #33046 PR-URL: #33048 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Zeyu Yang <himself65@outlook.com>

Node.js is currently considering printing a warning when a non-existent property of `module.exports` is accessed while in a circular `require()` dependency, in order to make it easier to catch issues with circular dependencies. In order to avoid printing these warnings for shelljs, checking for the property’s existence rather than its truthiness suffices. Refs: nodejs/node#29935

addaleax added the module Issues and PRs related to the module subsystem. label Oct 11, 2019

fixup! module: warn on using unfinished circular dependency

dd1cdfa

targos approved these changes Oct 11, 2019

View reviewed changes

fixup! module: warn on using unfinished circular dependency

3ba4313

addaleax mentioned this pull request Oct 11, 2019

fix: resolve circular dependency in pack.js npm/cli#266

Closed

addaleax mentioned this pull request Oct 18, 2019

Silence potentially upcoming circular dependency warning shelljs/shelljs#973

Merged

addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 23, 2019

addaleax added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Oct 29, 2019

addaleax closed this Nov 1, 2019

addaleax deleted the circular-dependency-warning branch November 1, 2019 19:15

BethGriggs mentioned this pull request Mar 10, 2020

2020-04-21, Version 14.0.0 (Current) #32181

Merged

This was referenced Apr 22, 2020

Node 14 with warning (node:31518) Warning: Accessing non-existent property 'column' of module exports inside circular dependency #32987

Closed

module: hide internal stack when emitCircularRequireWarning #32991

Closed

raymondfeng mentioned this pull request Apr 23, 2020

CLI prints a bunch of node warnings on Node.js 14.0.0 loopbackio/loopback-next#5203

Closed

patrick-rodgers mentioned this pull request Apr 23, 2020

Failing due to Winston dependency in Node 14 flatiron/prompt#199

Open

addaleax mentioned this pull request Apr 24, 2020

module: do not warn when accessing __esModule of unfinished exports #33048

Closed

3 tasks

AdrianoCahete mentioned this pull request May 8, 2020

Lots of warnings in CLI stylus/stylus#2533

Closed

This was referenced Jun 7, 2020

Bump minimist from 1.1.1 to 1.2.3 sintaxi/surge#402

Closed

Warning is output with Node.js 14 sintaxi/surge#403

Closed

tvvignesh mentioned this pull request Aug 29, 2020

Warnings during service:push in Node v14.8.0 with the CLI apollographql/apollo-tooling#2100

Open

iamareebjamal mentioned this pull request Nov 9, 2020

Upgrade to shelljs 0.8.4 Cropster/ember-l10n#86

Closed

MrWook mentioned this pull request Feb 5, 2021

Node 14 circular dependency typegoose/mongodb-memory-server#433

Closed

cheempz mentioned this pull request Feb 9, 2021

AO-18716 fix node 14 non-existent property warning solarwinds/appoptics-apm-node#139

Merged

broofa mentioned this pull request May 6, 2022

Importing nib spews warnings about "Accessing non-existent property '<property>' of module exports inside circular dependency" stylus/nib#356

Closed

module: warn on using unfinished circular dependency by addaleax · Pull Request #29935 · nodejs/node · GitHub

Uh oh!

Uh oh!

module: warn on using unfinished circular dependency - #29935

module: warn on using unfinished circular dependency#29935addaleax wants to merge 3 commits intonodejs:masternodejs/node:masterfrom addaleax:circular-dependency-warningaddaleax/node:circular-dependency-warningCopy head branch name to clipboard

Conversation

addaleax commented Oct 11, 2019

Checklist

Uh oh!

devsnek commented Oct 11, 2019

Uh oh!

addaleax commented Oct 11, 2019

Uh oh!

devsnek commented Oct 11, 2019

Uh oh!

addaleax commented Oct 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 11, 2019 • edited by addaleax Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boneskull commented Oct 11, 2019

Uh oh!

guybedford commented Oct 11, 2019

Uh oh!

targos commented Oct 11, 2019

Uh oh!

targos commented Oct 11, 2019

Uh oh!

addaleax commented Oct 11, 2019

Uh oh!

addaleax commented Oct 11, 2019

Uh oh!

addaleax commented Oct 11, 2019

Uh oh!

addaleax commented Oct 11, 2019

Uh oh!

DerekNonGeneric commented Oct 11, 2019

Uh oh!

addaleax commented Oct 11, 2019

Uh oh!

addaleax commented Oct 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tniessen commented Oct 20, 2019

Uh oh!

nodejs-github-bot commented Oct 23, 2019

Uh oh!

addaleax commented Oct 23, 2019

Uh oh!

nodejs-github-bot commented Oct 29, 2019

Uh oh!

addaleax commented Nov 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

module: warn on using unfinished circular dependency#29935
addaleax wants to merge 3 commits into
nodejs:masterfrom
addaleax:circular-dependency-warning

addaleax commented Oct 11, 2019 •
edited
Loading

nodejs-github-bot commented Oct 11, 2019 •
edited by addaleax
Loading

addaleax commented Oct 18, 2019 •
edited
Loading