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

feat: add config.operation #12298

Closed
wants to merge 3 commits into from
Closed

Conversation

brillout
Copy link
Contributor

@brillout brillout commented Mar 5, 2023

Description

This is solving a long standing problem: there isn't a reliable way to distinguish between dev and preview (since config.command === 'serve' for both $ vite dev and $ vite preview).

There are workarounds but none of them are reliable. (E.g. checking whether configureServer()/configurePreviewServer() is called doesn't work in certain situations.)

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 PR Title 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. N/A.

@stackblitz
Copy link

stackblitz bot commented Mar 5, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@benmccann
Copy link
Collaborator

It's really a shame we can't call this command as it's quite confusing to have command be 'build'|'serve' and they feel a bit swapped. I wonder if we should name this new one cmd and deprecate the old command?

@brillout
Copy link
Contributor Author

brillout commented Mar 6, 2023

It's really a shame we can't call this command as it's quite confusing to have command be 'build'|'serve' and they feel a bit swapped.

Yes, I couldn't agree more.

I wonder if we should name this new one cmd

👍 Done.

and deprecate the old command?

I think so too. How about we already start showing a warning in the next patch release?

@bluwy bluwy changed the title fix: add config.operation feat: add config.operation Apr 1, 2023
@bluwy bluwy added the p2-to-be-discussed Enhancement under consideration (priority) label Apr 1, 2023
@sapphi-red
Copy link
Member

I noticed that this change is a breaking change as it requires users to pass an additional argument to resolveConfig/loadConfigFromFile. We can avoid that by making the argument optional, but I fear that would lead to a same situation with configEnv.ssrBuild (#13809).

@bluwy
Copy link
Member

bluwy commented Nov 1, 2023

We discussed about this in the meeting today. To minimize the changes, and since the main usecase is to only differentiate dev and preview, we think passing a isPreview property to ConfigEnv (the second param of the config hook) should be enough for now. resolveConfig can accept a new parameter to pass in the isPreview value.

This would make it work similar to the ssrBuild value today. ssrBuild is used to differentiate what kind of build is running, and isPreview would be used to differentiate what kind of server is running.

@brillout
Copy link
Contributor Author

brillout commented Nov 2, 2023

Neat, good idea. I'll try to update the PR later today.

@bluwy
Copy link
Member

bluwy commented Nov 2, 2023

@brillout I've already opened a PR 😬 #14855

I forgot to credit you as a co-author for kicking this off though. Let me send a comment there so we do so before merging.

@brillout
Copy link
Contributor Author

brillout commented Nov 2, 2023

Neat. No need for credits.

@brillout brillout closed this Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change p2-to-be-discussed Enhancement under consideration (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants