Skip to content

set concurrency to 1 #3

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.

Sign up for GitHub

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

Open
toomu wants to merge 1 commit into
base: master
Choose a base branch
from
Open

set concurrency to 1 #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@ npm-debug.log
/node_modules
/example/installed
/example/data
.idea/
Copy link
Owner

@avaer avaer May 11, 2017

Choose a reason for hiding this comment

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

This seemingly has nothing to do with setting concurrency to 1, so it shouldn't be in this PR right?

5 changes: 3 additions & 2 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ const rollupPluginJson = require('rollup-plugin-json');
const watchr = require('watchr');
const cryptoutils = require('cryptoutils');
const MultiMutex = require('multimutex');
var bluebird = require('bluebird');
Copy link
Owner

@avaer avaer May 11, 2017

Choose a reason for hiding this comment

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

Can we avoid this bluebird dependency? This seems a huge thing to use when the equivalent promise looper is probably 5 lines of code.

Also we have the multimutex module already included, which solves this exact concurrency problem.


const defaultConfig = {
hostname: 'archae',
Expand Down Expand Up @@ -1163,7 +1164,7 @@ class ArchaeInstaller {
}
};
const _npmInstall = (modules, moduleNames, cb) => {
Promise.all(modules.map((module, index) => {
bluebird.map(modules, (module, index) => {
const moduleName = moduleNames[index];

const _ensureNodeModules = (module, moduleName) => new Promise((accept, reject) => {
Expand Down Expand Up @@ -1259,7 +1260,7 @@ class ArchaeInstaller {
return _ensureNodeModules(module, moduleName)
.then(() => _install(module, moduleName))
.then(() => _build(module, moduleName));
}))
}, {concurrency: 1})
.then(() => {
cb();
})
Expand Down
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@
},
Copy link
Owner

@avaer avaer May 11, 2017

Choose a reason for hiding this comment

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

The npm version probably needs to change to please package manager and deployment systems.

"repository": {
"type": "git",
"url": "https://github.com/modulesio/archae.git"
"url": "https://github.com/toomu/archae.git"
Copy link
Owner

@avaer avaer May 11, 2017

Choose a reason for hiding this comment

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

Was it deliberate to take over the repo in npm?

Copy link
Author

@toomu toomu May 15, 2017

Choose a reason for hiding this comment

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

I dont have permissions to write to https://github.com/modulesio/archae.git . Thought that implied I need to fork it to make the changes

Copy link
Owner

@avaer avaer May 15, 2017

Choose a reason for hiding this comment

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

Yeah, but with this change the whole repo would point to toomu/archae from now on in npm. I'm really not sure what the point of that would be.

If this is for testing, it probably shouldn't be committed, or at least should not be part of a PR.

Though I'm not quite sure how this helps in testing in the first place...

},
"dependencies": {
"bluebird": "3.5.0",
"autows": "0.0.5",
"cryptoutils": "0.0.7",
"etag": "^1.8.0",
Expand Down