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 isPreview to ConfigEnv and resolveConfig #14855

Merged
merged 5 commits into from
Nov 3, 2023
Merged

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Nov 2, 2023

Description

Add isPreview to ConfigEnv. Supersedes #12298

I made a very direct breaking change to ssrBuild (renamed to isSsrBuild) and we could decide if it's a good idea or not 😬

This will break loadConfigForFile mostly and its first parameter accepts a ConfigEnv. However, I don't think it's hard to fix it.

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@bluwy bluwy added p2-nice-to-have Not breaking anything but nice to have (priority) breaking change labels Nov 2, 2023
Copy link

stackblitz bot commented Nov 2, 2023

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

@patak-dev
Copy link
Member

For reference, we also use ssr instead of isSsr in other places. Maybe because of the sSs looking a bit strange there?
That being said, I'm good with the rename. I don't think this will be an issue when upgrading.

@bluwy
Copy link
Member Author

bluwy commented Nov 2, 2023

Yeah I also find the sSs a bit odd too, but with isPreview beside it I think it looks more consistent now.

@bluwy bluwy mentioned this pull request Nov 2, 2023
9 tasks
@bluwy
Copy link
Member Author

bluwy commented Nov 2, 2023

Before merging, make sure to set the co-author!

Co-authored-by: Romuald Brillout <git@brillout.com>

@brillout
Copy link
Contributor

brillout commented Nov 2, 2023

Very neat. (I've used many ugly hacks in the past to workaround this.)

set the co-author!

No need to :)

/**
* @experimental
*/
ssrBuild?: boolean
isSsrBuild: boolean
Copy link
Member

Choose a reason for hiding this comment

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

What is the plan for avoiding #8912? (https://discord.com/channels/804011606160703521/831456449632534538/993429210867707914)
For example, are we going to say "call loadConfigFromFile for each build separately"?
Otherwise, I guess vitepress won't be able to pass the correct value.

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea I'm thinking is that, if you don't know or can't be sure what it is, you can simply set false (we do the same for vitestSetup.ts in this PR). Maybe it makes more sense to allow undefined to convey the "i don't know part"?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've pushed a commit that makes them undefined-able

Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

I was thinking we could keep a ssrBuild getter with a warning to guide folks in the transition, but given the feature is rarely used I think we could merge this one with out that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants