fix: reintroduce watch and direct import of config files #2634
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`engine-cli` used to have 3 config modes: 1. require - was actually doing a direct `import()` to the config (was a `require()` call before the move to esm) 2. watch - bundles config files into cjs bundles. this was (and still is) broken when the config module, or any of the files it imports use esm-specific syntax (e.g import.meta). 3. fresh - this used `import()` via a freshly created worker_thread. it allowed getting the "current" value of a config file (in dev time), meaning you don't have to watch. it got broken when we moved to esm. #2629 removed the first two and fixed the third. this caused a speed regression downstream, as tests imported configs over and over in a non-cached manner. with this PR, we now have 3 modes again: 1. "import" - same functionality as before, but a non-confusing name 2. "watch" - same functionality as before. still broken when using native esm syntax 3. fresh - got fixed so it actually works again. not used unless cli flag is passed "import" is now the default unless cli is in watch mode (where "watch" gets be the default)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
engine-cliused to have 3 config modes:require- was actually doing a directimport()to the config (was arequire()call before the move to esm)watch- bundles config files into cjs bundles. this was (and still is) broken when the config module, or any of the files it imports use esm-specific syntax (e.g import.meta).fresh- this usedimport()via a freshly created worker_thread. it allowed getting the "current" value of a config file (in dev time), meaning you don't have to watch. it got broken when we moved to esm.#2629 removed the first two and fixed the third. this caused a speed regression downstream, as tests imported configs over and over in a non-cached manner.
with this PR, we now have 3 modes again:
import- same functionality as before, but a non-confusing namewatch- same functionality as before. still broken when using native esm syntaxfresh- got fixed so it actually works again. not used unless cli flag is passedimportis now the default unless cli is in watch mode (wherewatchgets be the default)