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

feat: reject if closed state is incorrect #42

Merged
merged 4 commits into from
Apr 15, 2024

Conversation

EvanHahn
Copy link
Contributor

@EvanHahn EvanHahn commented Apr 5, 2024

Some methods, like addCore(), now throw if called after calling close(). unlink() is the opposite, and should only be called after the indexer is closed.

This is arguably a breaking change, but I feel that changing undefined behavior is not breaking.

Closes #21.

Some methods, like `addCore()`, now throw if called after calling
`close()`. `unlink()` is the opposite, and should only be called after
the indexer is closed.

This is arguably a breaking change, but I feel that changing undefined
behavior is not breaking.
@@ -139,14 +139,25 @@ Type: `Hypercore`
Add a hypercore to the indexer. Must have the same value encoding as other
hypercores already in the indexer.

Rejects if called after the indexer is closed.

### indexer.idle()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method was previously undocumented. Because it has a behavior change, I documented it here.

@@ -12,3 +12,13 @@ function pDefer() {
return deferred
}
exports.pDefer = pDefer

class ExhaustivenessError extends Error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy-pasted from mapeo-core-next.

index.js Show resolved Hide resolved
Copy link
Member

@achou11 achou11 left a comment

Choose a reason for hiding this comment

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

non-blocking comments but lgtm

index.js Show resolved Hide resolved
index.js Outdated
* Stop the indexer and flush index state to storage. This will not close the
* underlying storage - it is up to the consumer to do that.
*
* Rejects if called more than once.
Copy link
Member

Choose a reason for hiding this comment

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

is this preferred over no-oping when closed already?

Copy link
Member

Choose a reason for hiding this comment

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

should update the docs with this note to describe the behavior anyways

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Double-closing is now a no-op in 56564f6. Updated the docs in d32e778.

@EvanHahn EvanHahn merged commit f56037a into main Apr 15, 2024
3 checks passed
@EvanHahn EvanHahn deleted the throw-if-not-in-correct-closed-state branch April 15, 2024 21:38
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.

Maintain closed state and add isClosed property
2 participants