Skip to content

Undefined options bug#688

Merged
arielsilvestri merged 3 commits intomasterfrom
undefined-options-bug
Nov 21, 2022
Merged

Undefined options bug#688
arielsilvestri merged 3 commits intomasterfrom
undefined-options-bug

Conversation

@arielsilvestri
Copy link
Copy Markdown
Contributor

@arielsilvestri arielsilvestri commented Nov 21, 2022

Addressing #649, there was a gap in the resolveArguments logic that wouldn't let options be set if properties were undefined, as the original logic did not account for the case where properties wouldn't be defined.

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Nov 21, 2022

🦋 Changeset detected

Latest commit: 5db6a3e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@segment/analytics-next Patch

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

expect(track.context).toEqual({ opt1: true })
})

test('sets context correctly if property arg is undefined', () => {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The bug didn't make it this far per se, but I wanted to harden this test suite again as it was a possible pain point.


let opts: Options = {}
if (isPlainObject(properties) && !isFunction(options)) {
if (!isFunction(options)) {
Copy link
Copy Markdown
Contributor Author

@arielsilvestri arielsilvestri Nov 21, 2022

Choose a reason for hiding this comment

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

Originally had isPlainObject(data) here as well but data will always be an object at this point so it felt unnecessary here.

@silesky
Copy link
Copy Markdown
Contributor

silesky commented Nov 21, 2022

Tested, this appears to work! Less complicated than I expected! Thanks!

Copy link
Copy Markdown
Contributor

@silesky silesky left a comment

Choose a reason for hiding this comment

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

Looks great, but missing a changeset!

@arielsilvestri arielsilvestri merged commit c21734e into master Nov 21, 2022
@arielsilvestri arielsilvestri deleted the undefined-options-bug branch November 21, 2022 21:52
@github-actions github-actions bot mentioned this pull request Nov 21, 2022
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.

2 participants