-
Notifications
You must be signed in to change notification settings - Fork 11.9k
build: re-enable latest version checks during release #29545
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
Conversation
07aec9a
to
622640b
Compare
// Root of the Angular CLI project. | ||
const root = fileURLToPath(new URL('../../../', import.meta.url)); | ||
const rootRequire = createRequire(root); | ||
const { latestVersions } = rootRequire('./dist/@schematics/angular/utility/latest-versions.js'); |
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.
Is dist/
guaranteed to be existent here? Seems like an assumption that is potentially brittle. Also the createRequire
is not super great :(
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.
Yes, the NPM packages are always built in the dist
, I did noticed that I was not reading from the release directory though which I amended.
const defaultDistPath = join(projectDir, 'dist/releases'); |
Regarding createRequire
, I agree that using a RegExp
could work. However, I lean toward this approach since it minimizes the risk of false positives or negatives.
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.
Yeah, definitely agreed on it being better than a RegExp.
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.
Ideally we'd somehow pass the packages and their artifact paths to the pre=release checks, but that's not possible right now, so LGTM
This commit restores the latest version checks, which are now fixed by using the stamped versions.
622640b
to
589f01e
Compare
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
This commit restores the latest version checks, which are now fixed by using the stamped versions.