lib: include cached modules in module.children by bnoordhuis · Pull Request #14132 · nodejs/node

bnoordhuis · 2017-07-08T11:27:57Z

module.children is supposed to be the list of modules included by this
module but lib/module.js failed to update the list when the included
module was retrieved from Module._cache.

Fixes: #7131
CI: https://ci.nodejs.org/job/node-test-pull-request/9038/

bnoordhuis · 2017-07-08T11:30:33Z

test/fixtures/GH-7131/a.js

I picked a module that's unlikely to have already been included by test/common.js.

refack · 2017-07-08T19:27:59Z

Worth a CitGM run: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/899/

Slayer95 · 2017-07-08T20:04:54Z

Does this mean that a structure a -> b -> a will be created if a and b are mutually dependent?

bnoordhuis · 2017-07-08T22:51:12Z

@Slayer95 Yes, there can be cycles now.

(That's the reason for the changes to test/sequential/test-module-loading.js: it didn't handle them.)

jasnell · 2017-07-10T16:33:48Z

test/parallel/test-module-children.js

path.join()?

Mwah, I can change that if you feel strongly but we have a gazillion tests that use simple string concatenation.

I'd prefer it. Updating the remaining ones for consistency would be a good Code-and-Learn activity (ping @nodejs/codeandlearn)

I'm not sure since https://nodejs.org/api/modules.html#modules_all_together (module names are URL like)
Maybe path.posix.join, but I'm +1 on literal /

@refack to be clear, you're saying that path.join() is wrong because it'll resolve to a\\b on Windows, and require always takes a POSIX style path (a/b)?

I'd rather have a concatenation (or `${common.fixturesDir}/GH-7131` ) than path.posix.join

@refack to be clear, you're saying that path.join() is wrong because it'll resolve to a\b on Windows, and require always takes a POSIX style path (a/b)?

Not wrong per se, just less right.

jasnell · 2017-07-10T16:34:05Z

test/parallel/test-module-children.js

path.join()?

bnoordhuis · 2017-07-17T21:22:55Z

path.join() nits included, new CI: https://ci.nodejs.org/job/node-test-pull-request/9202/

refack · 2017-07-18T02:24:31Z

path.join() nits included, new CI: https://ci.nodejs.org/job/node-test-pull-request/9202/

I have to kill the ARM job https://ci.nodejs.org/job/node-test-binary-arm/9302/ It was Green (yellow) but wasn't reporting that it finished upstream.

jasnell · 2017-07-18T04:39:58Z

CI appears to be having some issues right now

bnoordhuis · 2017-07-18T10:58:44Z

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

refack · 2017-07-18T11:09:38Z

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

FWIW https://ci.nodejs.org/job/node-test-binary-arm/9302/ was green, except for a flake in freeBSD. And currently CI is even flakier then last night.

bnoordhuis · 2017-07-19T11:23:26Z

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

bnoordhuis · 2017-07-19T11:24:47Z

FWIW https://ci.nodejs.org/job/node-test-binary-arm/9302/ was green, except for a flake in freeBSD. And currently CI is even flakier then last night.

@refack Are you saying that the whole run was green (except for a flake), not just the ARM machines? The node-test-binary-arm in the URL threw me off.

refack · 2017-07-19T12:29:15Z

@refack Are you saying that the whole run was green (except for a flake), not just the ARM machines? The node-test-binary-arm in the URL threw me off.

Yes. Whole run was green.

`module.children` is supposed to be the list of modules included by this module but lib/module.js failed to update the list when the included module was retrieved from `Module._cache`. Fixes: nodejs#7131 PR-URL: nodejs#14132 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

`module.children` is supposed to be the list of modules included by this module but lib/module.js failed to update the list when the included module was retrieved from `Module._cache`. Fixes: #7131 PR-URL: #14132 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

MylesBorins · 2017-08-16T07:57:08Z

@bnoordhuis should this be backported to LTS? if so how far back?

edit: considering this broke some stuff in the wild it might be best to just leave it out... at least of v4.x

Matchmaker is now globally accessible under Ladders, rather than needing to be manually required. This fixes a crash when hotpatching certain things.

Slayer95 · 2017-09-02T02:48:33Z

Confirming that this did break stuff at least for our project, which had to be dealt with using a global state-based quick&dirty fix.

MylesBorins · 2017-09-19T17:15:41Z

@nodejs/tsc
thoughts regarding the breakage mentioned above

MylesBorins · 2017-10-16T21:23:50Z

ping @nodejs/tsc and @nodejs/lts one more time

mcollina · 2017-10-16T21:27:15Z

-1 in backporting

kumarrishav · 2017-12-08T09:25:38Z

It's breaking this too. because of this . https://github.com/krakenjs/freshy/blob/master/index.js#L85

nodejs-github-bot added the module Issues and PRs related to the module subsystem. label Jul 8, 2017

bnoordhuis mentioned this pull request Jul 8, 2017

module.children doesn't contain cached modules #7131

Closed

bnoordhuis commented Jul 8, 2017

View reviewed changes

test/fixtures/GH-7131/a.js Outdated

Copy link

Member Author

bnoordhuis Jul 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I picked a module that's unlikely to have already been included by test/common.js.

refack approved these changes Jul 8, 2017

View reviewed changes

cjihrig approved these changes Jul 9, 2017

View reviewed changes

tniessen approved these changes Jul 9, 2017

View reviewed changes

jasnell reviewed Jul 10, 2017

View reviewed changes

test/parallel/test-module-children.js Outdated

Copy link

Member

jasnell Jul 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

path.join()?

tniessen reacted with thumbs up emoji

bnoordhuis force-pushed the fix7131 branch from 9a2e5e4 to 10827c2 Compare July 17, 2017 21:18

jasnell approved these changes Jul 17, 2017

View reviewed changes

refack approved these changes Jul 17, 2017

View reviewed changes

bnoordhuis force-pushed the fix7131 branch from 10827c2 to c83d9bb Compare July 24, 2017 15:00

bnoordhuis closed this Jul 24, 2017

bnoordhuis deleted the fix7131 branch July 24, 2017 15:00

bnoordhuis merged commit c83d9bb into nodejs:master Jul 24, 2017

addaleax mentioned this pull request Jul 24, 2017

v8.3.0 proposal #14452

Closed

addaleax mentioned this pull request Aug 2, 2017

v8.3.0 proposal #14594

Merged

mikermcneil mentioned this pull request Aug 15, 2017

Sails 1.0.0-33 doesn't boot on Node 8.3.0 when accessing require('sails').config balderdashy/sails#4165

Closed

MylesBorins added lts-watch-v4.x labels Aug 16, 2017

Slayer95 referenced this pull request in smogon/pokemon-showdown Sep 2, 2017

Fix Matchmaker hotpatching

7a9310a

Matchmaker is now globally accessible under Ladders, rather than needing to be manually required. This fixes a crash when hotpatching certain things.

MylesBorins added dont-land-on-v6.x and removed lts-watch-v4.x labels Oct 16, 2017

bnoordhuis mentioned this pull request May 2, 2019

JSON.stringify on process.nodeModules can result in an OOM when using custom serializer #27536

Closed

Uh oh!

Conversation

bnoordhuis commented Jul 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Jul 8, 2017

Uh oh!

Slayer95 commented Jul 8, 2017

Uh oh!

bnoordhuis commented Jul 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Jul 17, 2017

Uh oh!

refack commented Jul 18, 2017

Uh oh!

jasnell commented Jul 18, 2017

Uh oh!

bnoordhuis commented Jul 18, 2017

Uh oh!

refack commented Jul 18, 2017

Uh oh!

bnoordhuis commented Jul 19, 2017

Uh oh!

bnoordhuis commented Jul 19, 2017

Uh oh!

refack commented Jul 19, 2017

Uh oh!

MylesBorins commented Aug 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Slayer95 commented Sep 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MylesBorins commented Sep 19, 2017

Uh oh!

MylesBorins commented Oct 16, 2017

Uh oh!

mcollina commented Oct 16, 2017

Uh oh!

kumarrishav commented Dec 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

bnoordhuis commented Jul 8, 2017 •

edited

Loading

MylesBorins commented Aug 16, 2017 •

edited

Loading

Slayer95 commented Sep 2, 2017 •

edited

Loading