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

Untap support & various fixes #19

Merged
merged 11 commits into from
Oct 27, 2021
Merged

Untap support & various fixes #19

merged 11 commits into from
Oct 27, 2021

Conversation

sugarmanz
Copy link
Collaborator

@sugarmanz sugarmanz commented Oct 21, 2021

Various fixes detailed in release notes below.

Todo:

  • Add tests
  • Add docs
  • Add release notes

Concerns

  • I still want to potentially change the interceptor vals to vars for consistency regarding the replacement of the mutable TapInfos. That is, if changing to vars is actually a good thing. I think it is, but some validation would be nice 😊

Guarding against the ConcurrentModificationException is important, so changing to var is fine as long as we guard what can modify it.

  • Naming of untap API. Does untap convey appropriate intent and usability?

untap naming is fine.

  • Tapping a hook requires a name for debugging purposes. Should we just use the name as the ID? I feel like, no, because this could be commonly overwritten. Take the case of some plugin that wants to be applied twice, applying it the second time will override the first hooks, most likely causing some undue strife. Tbh, the whole name thing is not really used usefully anyways, and was only added as a direct port from JS tapable.

For now, keeping name and ID as they both have unique purposes.

  • Verification of the coroutine changes I've made -- though I'm pretty solid on those. I'd love to get rid of the Parallelism class at some point, but that can be done later.

Verified with a small fix.

Release Notes

Small fixes

  • Fix Gradle generation params
  • Modify async hook strategy to not take a scope, as this is already required to call the suspend method
  • Fix AsyncParallelHook to actually suspend properly until all callbacks complete
  • Replace mutable list containing TapInfo with a mutable var containing an immutable list (this fixes an issue when tapping a hook that is currently being called: ConcurrentModificationException)

Untapping

In order to allow calling sites to unregister stale callbacks and prevent memory leaks, the tap API now returns a String representing the ID of the specific "tap". The ID can then be passed into the new untap API to remove the callback from the hook. This ID can be randomly generated or manually passed when tapping a hook. Manually passing an ID is useful for when the tapper wants to replace a stale callback without calling needing to untap explicitly.

Version

Published prerelease version: 0.10.4-next.2

Changelog

⚠️ Pushed to next

Authors: 1

@sugarmanz sugarmanz marked this pull request as ready for review October 27, 2021 02:38
@sugarmanz sugarmanz added the minor Increment the minor version when merged label Oct 27, 2021
@sugarmanz sugarmanz changed the title Next Untap support & various fixes Oct 27, 2021
@sugarmanz sugarmanz merged commit 83eebf6 into main Oct 27, 2021
@sugarmanz sugarmanz deleted the next branch October 27, 2021 02:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Increment the minor version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant