Skip to content
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.

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

fix if condition to trigger the glues #61

Merged
merged 2 commits into from Jun 9, 2021
Merged

fix if condition to trigger the glues #61

merged 2 commits into from Jun 9, 2021

Conversation

staltz
Copy link
Member

@staltz staltz commented Jun 8, 2021

I think this is what is causing ssb-ebt tests to break, because ssb-ebt tests don't specify config.friends and then the glues were not running.

index.js Outdated
@@ -57,10 +57,10 @@ exports.init = function (sbot, config) {
const legacy = _legacy(layered)

// glue modules together
if (config.friends && config.friends.hookAuth !== false)
if (!config.friends || config.friends.hookAuth !== false)
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 think what you wanted to write here was just
if (config.friends.hookAuth !== false)
but you had to protect against config.friends being undefined.

The correct way is if (!config.friends || because we want the default to be "run the glue", even if config.friends is undefined.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, default values. Right. Hmm, I just find these kind of logic expressions hard to read.

Copy link
Member

Choose a reason for hiding this comment

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

That is a good find, I was a breaking change before...

Copy link
Member

Choose a reason for hiding this comment

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

maybe // default: true comment on this line?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure

Copy link
Member Author

Choose a reason for hiding this comment

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

Fun fact: ?. optional chaining and ?? nullish coalescing are already standardized, supported in all green browsers, and supported in Node.js 14:

var a = {}
console.log(a?.foo?.bar ?? 'yo')
// 'yo' 

Sometime in the next few years we can start using it to create defaults.

friends: {
hookAuth: false,
hookReplicate: false
},
Copy link
Member Author

Choose a reason for hiding this comment

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

These changes are required because the tests were relying on the glue code not running (because we don't have ssb-replicate installed, which the glues need). So here we have to opt-out.

@staltz staltz requested a review from arj03 June 8, 2021 13:55
@staltz
Copy link
Member Author

staltz commented Jun 9, 2021

@arj03 Ready for another look.

Copy link
Member

@arj03 arj03 left a comment

Choose a reason for hiding this comment

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

Very nice!

@staltz staltz merged commit 3dae207 into master Jun 9, 2021
@staltz staltz deleted the fix-glue-if branch June 9, 2021 07:52
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.

None yet

2 participants