Remove current version from type tests #26050
Open
+0
−1
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.
Description
Since the "current version" isn't a released/stable thing, putting it in the type tests could be a bit confusing, and can become actually wrong once that version is released.
Also including it makes version bumps touch all the type tests and require a lot of review, which doesn't seem useful.
This removes that line from the generated type tests.
The "Baseline (previous) version" might be useful (though is redundant with the version included in the package.json file), as that is an actual released version, and indicates which package version the type tests were generated from. This line has been kept. This version is low cost from a review/churn overhead as it changes at the same time as the rest of the type test file since it indicates which version it was generated from.
If there is a reason to keep this information, please indicate why its included either in the generated comment containing it and/or in the source code of the generation which is putting it in the file.
If the reason for it is to try and make it easier to avoid accidently having stale type-test files (when packages are renamed or type tests disabled), I think we should instead simply ensure that our build (which regenerates them on every run) actually handles the case where type tests are disabled correctly (by removing the file, or making a dummy one, or validating that we don't have
.generatedwhich would not be recreated by a build): this would be a robust solution, rather than hoping devs happen to notice that the version in a comment in the type test file is not the current version. Should we not have capacity to do that, we should at least document putting it in the generate file as a temporary low quality workaround, and maybe generate something more robust like this instead:(That would require pkgVersion to be included in all packages with type tests, which might be a good idea anyway.
Or generating runtime assert that loads the version from the package.json comparing it to the hard coded constant (avoids requiring the packageVersion file), and possible some other validation (could validate the file name is up to date, type tests enabled etc, but I really don't think it should be up to generated files to ensure their generator ran correctly: we should handle that as part of the build/generation of them so we can also validate things like packageVersion.js isn't out of date)
Reviewer Guidance
The review process is outlined on this wiki page.