Skip to content
This repository has been archived by the owner on Dec 5, 2019. It is now read-only.

fix(uglify/Runner): prevent errors on limited shells where os.cpus returns undefined #338

Closed
wants to merge 2 commits into from

Conversation

vielhuber
Copy link

No description provided.

@jsf-clabot
Copy link

jsf-clabot commented Aug 8, 2018

CLA assistant check
All committers have signed the CLA.

this.maxConcurrentWorkers = parallel === true ? os.cpus().length - 1 : Math.min(Number(parallel) || 0, os.cpus().length - 1);
// https://github.com/nodejs/node/issues/19022
// in some cases cpus() returns undefined
const cpusLength = (os.cpus() ? os.cpus().length : 1);
Copy link
Member

Choose a reason for hiding this comment

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

Let's rewrite to const cpusLength = (os.cpus() || { length: 1 }).length as said nodejs/node#19022 (comment)

Copy link
Author

Choose a reason for hiding this comment

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

Do I have to make another pull request for this change?

Copy link
Member

Choose a reason for hiding this comment

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

@vielhuber no, let's do here

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I don't know how this is done. Can you help out?

Copy link
Member

Choose a reason for hiding this comment

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

@vielhuber change code in you branch, commit changes and push with force. Sorry i don't have time to learn, better read book about git. You can create new PR if you don't know how do this.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@michael-ciniawsky michael-ciniawsky changed the title Prevent errors on limited shells where cpus returns undefined fix(uglify/Runner): prevent errors on limited shells where os.cpus returns undefined Aug 9, 2018
Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

Can you elaborate on which 'limited shell' your're currently using this plugin (and webpack). nodejs/node#19022 only seems to mention android (?). I'm not sure we nor node itself 'offically' supports that platform. Not blocking on this at all, just curious :)

this.maxConcurrentWorkers = parallel === true ? os.cpus().length - 1 : Math.min(Number(parallel) || 0, os.cpus().length - 1);
// https://github.com/nodejs/node/issues/19022
// in some cases cpus() returns undefined
const cpusLength = (os.cpus() || { length: 1 }).length;
Copy link
Member

Choose a reason for hiding this comment

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

const cpusLength => const cpus (🐦 nitpick)

@alexander-akait
Copy link
Member

alexander-akait commented Aug 9, 2018

@michael-ciniawsky some people use chroot when using linux, problem really exists in node and it is break plugin (i tested this locally and confirm problem exists). Explanation is not required. Why post about android, because many version of Anroid use chroot technology. Also Android is linux and nodejs should work good.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants