Skip to content

Conversation

@toomu
Copy link

@toomu toomu commented May 11, 2017

No description provided.

Copy link
Owner

@avaer avaer left a comment

Choose a reason for hiding this comment

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

This seems to regress the core parallel install functionality of this module. If we're going to add concurrency configuration, it should be taken in as an argument, not hard coded to 1. Otherwise a large part if the point of using archae in the first place is lost.

/node_modules
/example/installed
/example/data
.idea/ No newline at end of file
Copy link
Owner

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?

const watchr = require('watchr');
const cryptoutils = require('cryptoutils');
const MultiMutex = require('multimutex');
var bluebird = require('bluebird');
Copy link
Owner

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.

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

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

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

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...

@@ -7,9 +7,10 @@
},
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants