Fix action() callbacks returning -1 corrupting the context - #265
Open
QuentinRoy wants to merge 3 commits into
Open
Fix action() callbacks returning -1 corrupting the context#265QuentinRoy wants to merge 3 commits into
action() callbacks returning -1 corrupting the context#265QuentinRoy wants to merge 3 commits into
Conversation
馃 Changeset detectedLatest commit: 550827e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
action() ignoring -1 return valuesaction() returning -1 to break context
action() returning -1 to break context action() callbacks returning -1 corrupting the context
Owner
|
Thanks! I'm AFK for another day, will review tomorrow. Looks good I think though. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
action()is supposed to ignore whatever the callback returns and keep the existing context (at least that's what the source tends to indicate, the docs seem outdated, c.f. #266).There is a particularly edge case where it breaks: if the callback returns
-1. The current implementation runs the result through!!~fn(ctx, ev) && ctx, and~-1becomes0, so the reducer returnsfalseinstead of the original context.I assume this somewhat surprising syntax was introduced to save bytes. Unless returning
-1from an action was intended as some kind of escape hatch to clear context, but I cannot think of any use case for it, and it is untested. So I suspect it is a bug. Fortunately, the fix is simple and even shorter (5 chars removed, 3 added, excluding white spaces):This change adds a regression test for that case and replaces the old reducer with the version above so
action()always returns the existing context after running the callback.Testing
npm testinpackages/corenpm run bundlesizeinpackages/core