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: re-run build when config files are changed in watch mode #11962

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

fi3ework
Copy link
Member

@fi3ework fi3ework commented Feb 7, 2023

Description

fix #11916

Additional context

I left a TODO in the returned watcher variable, when the build function is re-runed, the watcher will be not be valid anymore. We need to add something like getWatcher: ()=> RollupWatcher(will be a breaking change) or use a global watcher ref to hold the reference. I'm not sure which is the better solution so I left a TODO there for code review. Anyway, with this PR, we could make the build re-run in cli.


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.

@fi3ework fi3ework force-pushed the re-run-build-function branch from 9c603ec to f482dc5 Compare February 7, 2023 06:03
@bluwy
Copy link
Member

bluwy commented Feb 19, 2023

I think we should make the build() function receive a new parameter to enable this for now, to prevent breaking programmatic usage. I also think returning a () => RollupWatcher instead might be nice API-wise if the parameter is enabled. We might also need to use overloads to prevent breaking the types.

In Vite 5, we can enable this by default. (We can add a TODO for that for now)

@bluwy bluwy added p3-minor-bug An edge case that only affects very specific usage (priority) feat: build labels Feb 19, 2023
@fi3ework fi3ework force-pushed the re-run-build-function branch from f482dc5 to 1c1916b Compare February 20, 2023 17:29
@fi3ework fi3ework force-pushed the re-run-build-function branch from 1c1916b to 9f8e407 Compare February 20, 2023 17:38
@fi3ework
Copy link
Member Author

fi3ework commented Feb 20, 2023

Made some modifications

  • the fix is enabled by default in cli mode
  • pass watchConfig param to enable this in the programmatic API
  • add a new ()=> RollupWatcher to the returned type of build, unsure if we need an overload to prevent the type breaking 🤔? Maybe we can run the /ecosystem-ci first to give it a try?

@fi3ework
Copy link
Member Author

Feeling like we need to add some type test to prevent casual public API breaking 🤔.

Comment on lines +239 to +245
/**
* Watch config files and restart the server when they changed.
* When enabled, the build function will return `() => RollupWatcher`
* This will be set to true by default and be removed in Vite 5.
* @default false
*/
watchConfig?: boolean
Copy link
Member

@bluwy bluwy Feb 25, 2023

Choose a reason for hiding this comment

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

I was thinking we add this param to build() directly since it only needs to be controlled programatically. That way it's easier to create the type overload too.

@@ -435,13 +443,17 @@ export async function resolveBuildPlugins(config: ResolvedConfig): Promise<{
}
}

let rollupWatcher: RollupWatcher
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to use a weakmap of the builds resolvedConfig to RollupWatcher, so that multiple build watches don't conflict with this single variable.

Comment on lines +664 to +665
'.env',
'.env.*',
Copy link
Member

Choose a reason for hiding this comment

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

The env files needs to be resolved from config.envDir too, instead of process.cwd() with path.resolve.

Comment on lines +673 to +676
configFileChokidar.on('all', async () => {
await Promise.all([watcher.close(), configFileChokidar.close()])
build(inlineConfig)
})
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would be nice to log that we're restarting because of config change, similar to

// auto restart server
debugHmr(`[config change] ${colors.dim(shortFile)}`)
config.logger.info(
colors.green(
`${path.relative(process.cwd(), file)} changed, restarting server...`,
),
{ clear: true, timestamp: true },
)

@bluwy
Copy link
Member

bluwy commented Feb 25, 2023

  • add a new ()=> RollupWatcher to the returned type of build, unsure if we need an overload to prevent the type breaking 🤔? Maybe we can run the /ecosystem-ci first to give it a try?

With the current PR, I think it would break since it returns a function-type regardless if ecosystem-ci passes, so it would be nice to avoid that with overloads.

Feeling like we need to add some type test to prevent casual public API breaking 🤔.

Yes that would be great too. We have a bit of types test currently at

"build-types-check": "tsx scripts/checkBuiltTypes.ts && tsc --project tsconfig.check.json",

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: build p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changes to vite.config.js build --watch mode is not reflected as mentioned in the docs
3 participants