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

feat(breaking change): experimental config options #749

Merged
merged 3 commits into from
Feb 1, 2017

Conversation

daviddias
Copy link
Member

@daviddias daviddias commented Feb 1, 2017

As recommended by @whyrusleeping, we need to pubsub behind a EXPERIMENTAL option in the config options.

Adding config options will cause a breaking cause in the API of creating a node, but it was a step we were going to make for #556 anyway.

@@ -0,0 +1,18 @@
exports.goOnline = require('./go-online')
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 not duplicate exports everywhere

module.exports = {
  goOnline: require('./go-online'),
  ...
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that way we have to worry about those commas, the way it is right now doesn't

Copy link
Member

Choose a reason for hiding this comment

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

Not sure what commas you mean

Copy link
Member Author

Choose a reason for hiding this comment

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

if it is inside an object, for every add/delete, we get the 'no dangling comma situation'

Copy link
Member

Choose a reason for hiding this comment

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

we have a lot of mixed and unclean diffs, not sure this makes any diff ;)

const node = new IPFS(repo)
const node = new IPFS({
repo: repo,
EXPERIMENTAL: {
Copy link
Member

Choose a reason for hiding this comment

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

Do we have to upcase this? I think experimental is enough. What would be good is to print something to the console when this is enabled.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like making things very OBVIOUS :D

Copy link
Member

Choose a reason for hiding this comment

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

BUT DO YOU REALLY NEED TO SCREAM AT ME?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add your log :)

Copy link
Member Author

Choose a reason for hiding this comment

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

added your log :)

@daviddias daviddias force-pushed the feat/put-experimental-behind-experimental branch from a533bee to 7ffcaf6 Compare February 1, 2017 12:35
@daviddias daviddias self-assigned this Feb 1, 2017
@daviddias daviddias merged commit 69fa802 into master Feb 1, 2017
@daviddias daviddias deleted the feat/put-experimental-behind-experimental branch February 1, 2017 13:15
@daviddias daviddias removed the status/in-progress In progress label Feb 1, 2017
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.

2 participants