@AlexChung1995 You need to get rid of these trailing spaces.

You could write a more concise/readable version using arrow functions and Array.includes:

if (!options.some(option => values.includes(option.value)))
  throw new Error('Option does not exist.');

But I'm still not sure of the correctness of this logic, I will comment shortly.

Correct me if I'm wrong, but I read this code as follows:

For every <select> option, try to find a corresponding entry in provided `values`.
If any of the options can't be matched, throw.

This is fundamentally different from what it should be doing:

For every provided `values`, try to find a corresponding entry in <select> options.
If any of the `values` can't be matched, throw.

I think the logic is equivalent. The code just attempts to match some value to some option in both cases. I think for readability, your version makes more sense with the expected error output so I will change it.

Getting rid of these 2 extra empty lines would be nice.

Ok, should I remove the old tests that fail now?

I think this needs to use values.every() instead of values.some() to handle variadic inputs.

Maybe also an additional test would be helpful, since it's not so trivial to reason about.

I suppose it would depend on the intended behaviour. I was more concerned about whether at least one change is made to the select element. With values.every(), the behaviour becomes concerned about whether all intended changes are made. If we use values.every() we would be required, I think, to include a list of options that were not selected with the error message.

I agree with having a special semantic for null would be a good way to include the old functionality of deselecting options. If I figure out a better way I will implement it.

I'm of the opinion that, if we are doing these checks, then they should have the same semantics for one or several elements. Or better yet, follow the existing heuristic: if the select has the multiple attribute set, check all provided values exists within the options, otherwise only check the first.

If your goal is to only know how many options were changed/selected, I would prefer a different approach where we don't throw and instead just return an int for the total number of selections made.

Perhaps @aslushnikov can weight in on this, since it's quite subjective.

I think the function can be considered to fail if it does not change the select element so would throw an error. That was why I thought throwing an error might be a good idea.

A return value is also a good solution.If the function was to return a value, perhaps an array of all selected elements would be the best. For the deselected functionality, the function could then return an empty array.

I find the heuristic of, on a select element without the multiple property set to true, assigning the element the first value in the provided array kind of counter intuitive. I could see two cases where a developer might provide multiple values to a select element that takes on only one value.
1.) the developer believes the select element can actually take on multiple values. If you believe this case is how the function will be used, I think it should throw an error because the developers intended usage is not what will actually happen.
2.) the developer provides multiple values, some of which may not be options, and wants the select element to set the first value that is an option.

What do you think? @aslushnikov @alixaxel

do i understand correctly, that you now return all the selected options instead of throwing an error? can we add this to the doc?

nit: let's update jsdoc's return

Promise.all doesn't make sense to me here: there are no promises inside the array

if we want to return values that are selected, i'd prefer generating them from options.

// ... 
return options.filter(option => option.selected).map(option => option.value);

nit: please elaborate when exactly it should throw

I don't understand why you need filteredValues. This line can be rewritten as the following:

option.selected = values.includes(option.value);

... and this line can be just

// explicitly assign `undefined` to state that passing in empty array resets selection.
element.value = values.length ? values[0] : undefined;

I thought it would be nice if the function would select the first value in values that also exists in options. I can do it in one loop like this if youd prefer.

element.value = undefined;
for (var i = values.length - 1; i>=0; i--){
    for (var option of options){
        if (option.value === values[i]){
            option.selected = true;
            break;
        }
    }
}

I thought it would be nice if the function would select the first value in values that also exists in options. I can do it in one loop like this if youd prefer.

Can you please clarify why do you want this? I don't think this is of much importance, so I'd rather have code as simple as possible.

I thought that was the only way it would make sense to pass multiple values to this function when the found select element only takes one value. You could create an array of values, order them by priority, and the function would select the value that exists with highest priority.

Otherwise, I think passing multiple values into this function when the select element only takes one value is a logical flaw. When someone just sets the select element to be the first value, if there is no such value in options it deselects everything. So you would have asked the function to select multiple options, but it actually deselects every option.

It does increase code complexity, so I understand if you disagree.

  async select(selector, ...values) {
    return await this.$eval(selector, (element, values) => {
      if (element.nodeName.toLowerCase() !== 'select')
        throw new Error('Element is not a <select> element.');
      const options = Array.from(element.options);
      element.value = undefined;
      for (let i = values.length - 1; i >= 0; i--){
          for (const option of options){
              if (option.value === values[i]){
                  option.selected = true;
                  break;
              }
          }
      }
      element.dispatchEvent(new Event('input', { 'bubbles': true }));
      element.dispatchEvent(new Event('change', { 'bubbles': true }));
      return options.filter(option => option.selected).map(option => option.value);
    }, values);
  }

@AlexChung1995 Wouldn't the following achieve the same effect you're proposing?

async select(selector, ...values) {
  return await this.$eval(selector, (element, values) => {
    if (element.nodeName.toLowerCase() !== 'select')
      throw new Error('Element is not a <select> element.');

    const options = Array.from(element.options);

    for (const option of options) {
      option.selected = values.includes(option.value);

      if ((option.selected === true) && (element.multiple === false)) {
        break;
      }
    }

    element.dispatchEvent(new Event('input', { 'bubbles': true }));
    element.dispatchEvent(new Event('change', { 'bubbles': true }));

    return options.filter(option => option.selected).map(option => option.value);
  }, values);
}

I tried that implementation and realized that it prioritizes by the order of the options and not the order of the values. I tried iterating from the start of the values array but thought the code was ugly. For example:

element.value = undefined;
for (const value of values) {
    const option = options.find(option => option.value === value);
    if (option){
        option.selected = true;
        if (!element.multiple)
            break;
    }
}

If you let me know which of the 3 versions you would prefer (original, prioritize by value, prioritize by option), Ill update the code and commit so we can wrap up this pull request!

@AlexChung1995 thanks, I understand now your intentions here.

Otherwise, I think passing multiple values into this function when the select element only takes one value is a logical flaw.

I agree. We shouldn't encourage this, so I'd avoid giving any special treatment to the multiple values passed in for a non-multiple select.

Given this, I like the second suggestion with dropped if-statement:

async select(selector, ...values) {
  return await this.$eval(selector, (element, values) => {
    if (element.nodeName.toLowerCase() !== 'select')
      throw new Error('Element is not a <select> element.');

    const options = Array.from(element.options);
    for (const option of options) 
      option.selected = values.includes(option.value);

    element.dispatchEvent(new Event('input', { 'bubbles': true }));
    element.dispatchEvent(new Event('change', { 'bubbles': true }));
    return options.filter(option => option.selected).map(option => option.value);
  }, values);
}

It's dummy, which is nice.

Ok, Ill do another commit. Since this is your solution @alixaxel I assume you're ok with it?
Thanks @alixaxel @aslushnikov for your help.

nit: do we need this?

Yeah, it fails tests otherwise.

nit: can you please update jsdoc on line 775? it should have the "return" statement

Like this?
- returns: <[Promise]<[Array]<[string]>>> Returns an array of option values that have been successfully selected.

I put that in there but its on line 928. Line 775 in my api.md file refers to Page.goForward().

I mean the jsdoc for this function (that block comment with @param tags inside)

oh i see

where can i find documentation about the !, ? and | modifiers for jsdoc? I dont know what they do.

@AlexChung1995 indeed. Briefly speaking, the "!" is a non-nullable type, the "?" is a nullable type. More info could be found here: https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the-Closure-Compiler