Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

Change default config from JSON file to JS module #1324

Conversation

Mr0grog
Copy link
Contributor

@Mr0grog Mr0grog commented Apr 23, 2018

Because the default node configuration was a JSON file, it was treated as a singleton object. In some parts of the code, data got added to it, leading to race conditions when creating multiple nodes simultaneously.

This is one approach to fixing it: to make creating a shared global object harder, the default config is now a function that returns a unique configuration object on each call. (The test is probably valuable whether or not we like this approach.)

Fixes #1316.

Because the default config was a JSON file, it was treated as a singleton object. In some parts of the code, data got added to it, leading to race conditions when creating multiple nodes simultaneously. To make that harder to do accidentally, the default config is now a *function* that returns a unique configuration object on each call.

Fixes ipfs#1316.

License: MIT
Signed-off-by: Rob Brackett <[email protected]>
@Mr0grog
Copy link
Contributor Author

Mr0grog commented Apr 23, 2018

(CI failures are from in-progress files API work, not this.)

daviddias
daviddias previously approved these changes Apr 24, 2018
Copy link
Member

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you @Mr0grog :)

@daviddias
Copy link
Member

You are correct, the failing tests are not due to this PR, but the linting fail is. Can you update your PR to make sure it passes linting?

License: MIT
Signed-off-by: Rob Brackett <[email protected]>
@Mr0grog
Copy link
Contributor Author

Mr0grog commented Apr 24, 2018

Ah! I hadn’t realized Jenkins was running different stuff than Travis and didn’t see those failures. Should be fixed now.

I also noticed some of these:

310:1   error  Trailing spaces not allowed  no-trailing-spaces

Would you mind if I submitted a PR for adding a .editorconfig file? Seems like it would be useful (one of the things you can set with it is removal of trailing spaces).

@daviddias
Copy link
Member

re: .editorconfig, I don't particularly mind, I just worry that then we have to maintain different configs for each editor, otherwise, contributors will then get confused when a rule changes. So far the burden has been on the contributor. I use ALE (https://github.com/w0rp/ale) with StandardJS and it does the job pretty well.

@daviddias
Copy link
Member

@Mr0grog also, added you to the JavaScript team on IPFS so that you can create branches on this repo, making collaboration easier. Right now, CI is failing because you need to rebase master onto this branch.

@daviddias
Copy link
Member

I'm merging this PR as the failures are unrelated and fixed on master. Let's discuss the editorconfig in parallel and in the future, please open branchs on this repo directly. Thank you!

@daviddias daviddias merged commit c3d2d1e into ipfs:master Apr 30, 2018
@Mr0grog Mr0grog deleted the fix/1316-two-peers-one-id-and-a-univeral-configuration branch April 30, 2018 17:06
@Mr0grog Mr0grog mentioned this pull request Apr 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Two peers with same Identity
2 participants