Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork378
fix: syncMode -default: useappend strategy when using dev serve…#887
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull Request Overview
This PR fixes thesyncMode: "default" behavior to properly useappend strategy during development andoverwrite strategy during build. Previously, the default mode was incorrectly usingoverwrite strategy when using the dev server.
- Introduces environment-based logic to determine the appropriate sync mode when
syncModeis set to"default" - Removes duplicate sync mode assignment that was overriding the intended behavior in dev server setup
- Adds clarifying comment explaining the default behavior logic
Tip: Customize your code reviews with copilot-instructions.md.Create the file orlearn how to get started.
src/core/context.ts Outdated
| this._removeUnused=this.options.syncMode==='overwrite' | ||
| // syncMode - `default`: use `append` strategy when using dev server, `overwrite` strategy when using build. | ||
| constsyncMode=this.options.syncMode==='default' ?(process.env.NODE_ENV==='development' ?'append' :'overwrite') :this.options.syncMode |
CopilotAISep 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[nitpick] The ternary operator is nested and difficult to read. Consider extracting this logic into a separate method likeresolveSyncMode() for better maintainability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Okay, I have fixed this issue and moved the logic to the options.ts file.
@xiaweiss IMO we should never relay on environment variables at this level. Development server being present does not mean running in Just have a look on previous implementation, it was using Therefore my opinion is that this changed may even break more use cases. If there's a bug we should have first a look at a reproduction that show something breaking when upgrading the version. I don't have more time for now to dive more in this, I'll let@antfu hopefully have a decision about what to do here. |
Thank you very much for your feedback. I will further research and explore how to determine the status of the dev-server. |
Uh oh!
There was an error while loading.Please reload this page.
…r, `overwrite` strategy when using build.
update: process.env.NODE_ENV has been deprecated, and replaced with the setupWatcher hook. |
commit: |
5794591 intounplugin:mainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
fix: syncMode -
default: useappendstrategy when using dev server,overwritestrategy when using build.Description
Setting is:
syncMode: "default"The
overwritewas executed when using dev server.Updated: PR handles the
defaultoption within thesetupWatcherhook, and dev-server usesappendby default.The PR has added an environment variable check to handle the default behavior of "default".Relevant Pull Request
feat: introduce syncMode to control the behavior of generating files