Add removedValue into onChange clear action meta

[eugenet8k]

The similar actions remove-value and pop-value have removedValue
in their onChange meta arguments.

It would be reasonable to pass the whole this.state.selectValue as
removedValue assuming that all selected values are removed upon clear.

This will help some client code to add additional logic in onChange
handlers like filtering of fixed values to preserve after clear without
referring to the options array.

[changeset-bot]

🦋 Changeset detected

Latest commit: 43c8330

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

This PR includes changesets to release 1 package

Name	Type
react-select	Minor

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

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 43c8330:

Sandbox	Source
react-codesandboxer-example	Configuration

[eugenet8k]

@gwyneplaine and @JedWatson I have rebased to the current master branch. This PR still seems reasonable IMO. I think it is good to follow existing ReactSelect API of passing items that are affected by current action in the event context.

[eugenet8k]

@Methuselah96 and @JedWatson please review. Despite all the recent improvements (thank you!) this commit still seems relevant and brings value IMO.

[Methuselah96]

Can you add a changeset?

[Methuselah96]

This will be an array, whereas the removedValue on the other actions only remove one option at a time, so we should probably make this plural so that the distinction is clear.

Suggested change

	this.onChange(null, { action: 'clear', removedValue: selectValue });
	this.onChange(null, { action: 'clear', removedValues: selectValue });

[eugenet8k]

@Methuselah96 I certainly see the logic behind your suggestion, but the existing selectValue is also an Array, so I thought it would be rather confusing to introduce removedValues and not having selectValues in a first place. As a developer that got to know with ReactSelect API recently, I would rather go with a single removedValue, that generally describes a Value (regardless of type) that is being removed. But if you guys insist I will update it.

[Methuselah96]

The selectValue in state is not exposed in the API, it's a private name, whereas this is in the public API.

I intend to do an internal refactor to rename selectValue to selectedOptions when I get the chance, but this would not be a breaking change, whereas renaming removedValue to removedValues would be a breaking change. That is why I'm inclined to prefer to get this name right up-front.

[eugenet8k]

@Methuselah96 thanks for the expanded explanation, this all make sense. I have pushed the updated commit.

[ebonow]

Sorry to play devil's advocate with semantics, but I struggle with this because this does introduce yet one more nuance. As a developer new to this library with all of the onChange actions, am I interested in value, option, removedValue, or removedValues?

removedValues would inaccurately be identified as plural if the Select is not multi.

One other possible fringe nuance comes to mind. Would it be possible to create a use case to remove multiple values at once, and if so, wouldn't the expected actionMeta for remove-value expect a removedValue prop, but then for clear becomes removedValues.

In my mind, a singular selectValue was cleared, so removedValue (or even possibly clearedValue) makes more sense for me. Philosophically, my preference is that the actionMeta object be more predictable instead of the user being responsible for knowing all the different object shapes for different actions.

I agree that the terminology used to describe a value is confusing and have secretly also desired to see it changed to the distinction selected so that options can just selected or unselected.

[Methuselah96]

In my mind you would always filter the action meta by the action property before you do anything with the other properties. As a developer I would look at the different shapes that an action meta can be and look at the appropriate properties for that action.

The distinction is that removedValues would always be an array (even if it's a single select). removedValue is always a single option. I think it's important to name them differently in order to know that you have to deal with them separately. If we did decide to go with removedValue it would be a breaking change because current code might be depending on the fact that removedValue is not an array.

I think we're all in general agreement that the current naming is confusing. Hopefully we can we work toward clarify it moving forward, but for right now I think this is the best we can do.

[eugenet8k]

@Methuselah96 @JedWatson please approve. All feedback was addressed.

[Methuselah96]

@eugenet8k Apologies for the delay on this and thank you for your patience. We're working on releasing a 4.0 in order to resolve #4094. I've added this to the 4.1 milestone and we'll review this PR to see if we want to include it in that release.

[Methuselah96]

@eugenet8k My apologies, but it looks like there are some merge conflicts with #4339 (that we merged for the 4.0 release). Would you mind resolving those conflicts? Hopefully we can get this merged in soon if the rest of the team is on board with the change.

resolved

[Kepro]

just a side note, typescript definition is wrong and also docs

  onChange(value, { action, removedValue }) {
    switch (action) {
      case 'remove-value':
      case 'pop-value':
        if (removedValue.isFixed) {
          return;
        }
        break;
      case 'clear':
        value = colourOptions.filter(v => v.isFixed);
        break;
    }

    value = orderOptions(value);
    this.setState({ value: value });
  }

it should be now

  onChange(value, { action, removedValue, removedValues }) {
    switch (action) {
      case 'remove-value':
      case 'pop-value':
        if (removedValue.isFixed) {
          return;
        }
        break;
      case 'clear':
        value = removedValues.filter(item => item.isFixed);
        break;
    }

    value = orderOptions(value);
    this.setState({ value: value });
  }

• created a PR for definition update
  [react-select] add removedValues to clearActionMeta DefinitelyTyped/DefinitelyTyped#52345