Ensure local deps are up-to-date when testing or running the CLI locally #930
Conversation
This file contains hidden or 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
ItsHarper
force-pushed
the
test/ensure-up-to-date-deps
branch
from
0f8ebbd to
278b751
Compare
While jest can run a package and its tests without needing a full build step, it will use the last-built version of the other packages for its dependencies, so we're not actually guaranteed to be testing the latest state unless we build all of a package's local dependencies before we start testing it. The `test:watch` scripts have been removed, as a watch mode that tests without building the dependencies is not reliable. I don't think any local packages are getting injected, but I added `syncInjectedDepsAfterScripts` to `pnpm-workspace.yaml` to be safe. Since we're running tests in parallel, individual tests run slower, so I had to increase some timeouts.
Replaces `codemod-dev` shell script with a new `cli` pnpm script in the top-level package
ItsHarper
force-pushed
the
test/ensure-up-to-date-deps
branch
from
278b751 to
1c790db
Compare
ItsHarper
changed the title
ItsHarper
changed the title
ItsHarper
changed the title
|
Is there anything that needs to happen before this is reviewable? |
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.
Context
Step 2 for #928. View isolated diff and CI results here: ItsHarper#5
At time of writing, the isolated diff has
+37, -57lines added/removed.This change is ready-to-go, but I'm marking it as a draft until #929 has been merged, as it is relatively difficult to review in this context until that happens.
Problem
While jest is set up to run a package and its tests without needing a full build step, it will use the last-built version of the other packages for its dependencies, so we're not actually guaranteed to be testing the all of the latest code unless we build all of a package's local dependencies before we start testing it. The same goes for the
codemod-devshell script.This problem doesn't exist when using
tsc, because there are references set up in each package'stsconfig.jsonfile. Unfortunately,esbuilddoesn't respect that as far as I can tell, so the issue does exist when running the tests orcodemod-dev(see evanw/esbuild#1250 for the only discussion I found of this discrepancy betweenesbuildandtsc).Solution
For the tests, I've added a top-level
checkscript that builds the whole project and then runs alltestandlintscripts from the packages in parallel. CI has been updated to use this instead of building, linting, and testing each package individually, and I wouldI've replaced
codemod-devwith a new top-levelclipnpm script that runs a build and then executes the resulting JS.The
test:watchscripts have been removed, as a watch mode that tests without ensuring the local dependencies are -up-to-date is not reliable. If they're deemed important enough, I can look into re-running the tests when the built files changed (combined withtsc --watch, that should be the complete picture).I don't think any local packages are getting injected, but I added
syncInjectedDepsAfterScripts: [ 'build' ]topnpm-workspace.yamlto be safe.Performance of local execution and tests
Always running the build step before running tests or the CLI obviously will involve a performance hit, and it seems like you at least used to care a lot about that. However, while a repeat build with no changes made takes about 2.4 seconds on my computer for this PR, that's reduced to just 1.8 seconds in #931 (which updates Typescript but makes no configuration changes), and about 650ms in #932.
The build should then get about 10x faster than that when Typescript 7 lands (which is written in go and takes advantage of multithreading). This should be a nearly a non-issue in practice, especially long-term (I'd expect Typescript 7 to be released within the next year, based on my perception of the speed of their progress).
Especially now that development has slowed so much on this project, prioritizing simplicity and correctness over dev-time performance seems like the right choice to me anyway.