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

fix(cli): when the user enters the same command #10474

Merged
merged 6 commits into from Oct 17, 2022
Merged

fix(cli): when the user enters the same command #10474

merged 6 commits into from Oct 17, 2022

Conversation

yucccc
Copy link
Contributor

@yucccc yucccc commented Oct 14, 2022

Description

When the user enters multiple identical options or incorrect option type, it can handle it correctly. fix: #10424

yarn vite --port --port="123" --host --host

Additional context

Fixed to: take the last array, and if you need to customize the output, you can correctly process the returned format.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@bluwy
Copy link
Member

bluwy commented Oct 15, 2022

This seems to be crossing the work for #10428. I think we should combine them similar to #10428 (comment). cc @btea

@btea
Copy link
Collaborator

btea commented Oct 15, 2022

I think @yucccc's processing method may be better, but he should also handle other options like preview option. WDYT? @yucccc @bluwy

? normalizeOptionsConfig[key](value)
: value[value.length - 1]
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

The logic here may be extracted and then applied to other options as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll try to modify it.

@@ -28,6 +28,27 @@ interface GlobalCLIOptions {
force?: boolean
}

const normalizeOptionsConfig: Record<string, (v: any[]) => any> = {
port: (v) => {
return v.find((p) => typeof p === 'number' || typeof p === 'string') || 5173
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe findLast should be used here? Because if the other options are arrays, the last value you select.

Copy link
Contributor Author

@yucccc yucccc Oct 15, 2022

Choose a reason for hiding this comment

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

ok

@bluwy
Copy link
Member

bluwy commented Oct 16, 2022

I think @yucccc's processing method may be better, but he should also handle other options like preview option. WDYT? @yucccc @bluwy

I think the "use last duplicated value" logic in this PR is good, but code pattern wise I quite prefer #10428 😬 We need to normalize/sanitize the options for the related commands, and cleanOptions() should mainly do props deletion only.

@yucccc
Copy link
Contributor Author

yucccc commented Oct 16, 2022

Now the logic has become pure, and no longer try to preset that the value of the last option does not meet the specification. Find the next valid value forward and don't know if it meets your expectations.

@sapphi-red sapphi-red mentioned this pull request Oct 17, 2022
9 tasks
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for tackling this @yucccc and @btea!

@patak-dev patak-dev merged commit 2326f4a into vitejs:main Oct 17, 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.

Vite Local address is badly formatted when using multiple --host
4 participants