Skip to content

Commit

Permalink
Remove as assertions, or fix to mitigate the risk of silent failure (…
Browse files Browse the repository at this point in the history 
…#158)

* Replace `as T[]` assertions with `?? []`

- `as` assertions intended to exclude an empty/nullable type can be replaced by a nullish coalescing operator converting nullable values into an acceptable empty value that doesn't pollute the variable's type signature.
- TODO: document and expound in MetaMask/contributor-docs#47

* Replace `as` assertions with redundant type guards

Sometimes TypeScript's type narrowing and inference doesn't fully work quite like it should. Even in these cases, using `as` assertions should be avoided.

It may seem like we know for certain what the type should be and there's no risk of it breaking, but there's always the possibility that some combination of changes elsewhere might affect the typing, in which case `as` will suppress the alerts TypeScript would otherwise provide us with. This may lead to code breaking silently.

In this case, adding type guards and null checks -- even if they're redundant -- is preferable to the above scenario.

- TODO: document and expound in MetaMask/contributor-docs#47

* Make `as` assertion rely on inference instead of hard-coded type casting

If anything changes in the typing of `ChangeCategory` or `unreleasedChanges` that make them inconsistent, this code will fail silently. Relying on type inference instead of explicit type casting will make these line a little less brittle.

- TODO: document and expound in MetaMask/contributor-docs#47

* Fix type narrowing for `currentVersion` by consolidating conditional branches

- more consistent logic for `isReleaseCandidate` null check
- wasted processing overhead due to errors throwing later than strictly necessary

* Install `@metamask/utils` as a devDep

* Replace `Object.keys` with `getKnownPropertyNames` method

- removes need for type casting while iterating over object keys

* Update src/update-changelog.ts
  • Loading branch information
@MajorLift
MajorLift authored Oct 17, 2023
1 parent d19ed72 commit 4804d3e
Show file tree
Hide file tree
Showing 4 changed files with 212 additions and 45 deletions.
1 change: 1 addition & 0 deletions package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
"@metamask/eslint-config-jest": "^11.1.0",
"@metamask/eslint-config-nodejs": "^11.1.0",
"@metamask/eslint-config-typescript": "^11.1.0",
"@metamask/utils": "^8.1.0",
"@types/cross-spawn": "^6.0.2",
"@types/diff": "^5.0.0",
"@types/jest": "^26.0.23",
Expand Down
9 changes: 5 additions & 4 deletions src/changelog.ts
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { getKnownPropertyNames } from '@metamask/utils';
import semver from 'semver';

import {
Expand Down Expand Up @@ -89,7 +90,7 @@ function stringifyRelease(
const categorizedChanges = orderedChangeCategories
.filter((category) => categories[category])
.map((category) => {
const changes = categories[category] as string[];
const changes = categories[category] ?? [];
return stringifyCategory(category, changes);
})
.join('\n\n');
Expand Down Expand Up @@ -383,11 +384,11 @@ export default class Changelog {

const unreleasedChanges = this.#changes[unreleased];

for (const category of Object.keys(unreleasedChanges) as ChangeCategory[]) {
for (const category of getKnownPropertyNames(unreleasedChanges)) {
if (releaseChanges[category]) {
releaseChanges[category] = [
...(unreleasedChanges[category] as string[]),
...(releaseChanges[category] as string[]),
...(unreleasedChanges[category] ?? []),
...(releaseChanges[category] ?? []),
];
} else {
releaseChanges[category] = unreleasedChanges[category];
Expand Down
77 changes: 36 additions & 41 deletions src/update-changelog.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -197,11 +197,6 @@ export async function updateChangelog({
tagPrefixes = ['v'],
formatter = undefined,
}: UpdateChangelogOptions): Promise<string | undefined> {
if (isReleaseCandidate && !currentVersion) {
throw new Error(
`A version must be specified if 'isReleaseCandidate' is set.`,
);
}
const changelog = parseChangelog({
changelogContent,
repoUrl,
Expand All @@ -215,15 +210,6 @@ export async function updateChangelog({
tagPrefixes,
});

if (
isReleaseCandidate &&
mostRecentTag === `${tagPrefixes[0]}${currentVersion || ''}`
) {
throw new Error(
`Current version already has tag, which is unexpected for a release candidate.`,
);
}

const commitRange =
mostRecentTag === null ? 'HEAD' : `${mostRecentTag}..HEAD`;
const commitsHashesSinceLastRelease = await getCommitHashesInRange(
Expand All @@ -247,38 +233,47 @@ export async function updateChangelog({
return undefined;
}

// Ensure release header exists, if necessary
if (
isReleaseCandidate &&
!changelog
.getReleases()
.find((release) => release.version === currentVersion)
) {
// Typecast: currentVersion will be defined here due to type guard at the
// top of this function.
changelog.addRelease({ version: currentVersion as Version });
}
if (isReleaseCandidate) {
if (!currentVersion) {
throw new Error(
`A version must be specified if 'isReleaseCandidate' is set.`,
);
}

if (isReleaseCandidate && hasUnreleasedChanges) {
// Typecast: currentVersion will be defined here due to type guard at the
// top of this function.
changelog.migrateUnreleasedChangesToRelease(currentVersion as Version);
}
if (mostRecentTag === `${tagPrefixes[0]}${currentVersion ?? ''}`) {
throw new Error(
`Current version already has a tag ('${mostRecentTag}'), which is unexpected for a release candidate.`,
);
}

const newChangeEntries = newCommits.map(({ prNumber, description }) => {
if (prNumber) {
const suffix = `([#${prNumber}](${repoUrl}/pull/${prNumber}))`;
return `${description} ${suffix}`;
// Ensure release header exists, if necessary
if (
!changelog
.getReleases()
.find((release) => release.version === currentVersion)
) {
changelog.addRelease({ version: currentVersion });
}
return description;
});

for (const description of newChangeEntries.reverse()) {
changelog.addChange({
version: isReleaseCandidate ? currentVersion : undefined,
category: ChangeCategory.Uncategorized,
description,
if (hasUnreleasedChanges) {
changelog.migrateUnreleasedChangesToRelease(currentVersion);
}

const newChangeEntries = newCommits.map(({ prNumber, description }) => {
if (prNumber) {
const suffix = `([#${prNumber}](${repoUrl}/pull/${prNumber}))`;
return `${description} ${suffix}`;
}
return description;
});

for (const description of newChangeEntries.reverse()) {
changelog.addChange({
version: isReleaseCandidate ? currentVersion : undefined,
category: ChangeCategory.Uncategorized,
description,
});
}
}

return changelog.toString();
Expand Down
170 changes: 170 additions & 0 deletions yarn.lock
View file
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,48 @@ __metadata:
languageName: node
linkType: hard

"@ethereumjs/common@npm:^3.2.0":
version: 3.2.0
resolution: "@ethereumjs/common@npm:3.2.0"
dependencies:
"@ethereumjs/util": ^8.1.0
crc-32: ^1.2.0
checksum: cb9cc11f5c868cb577ba611cebf55046e509218bbb89b47ccce010776dafe8256d70f8f43fab238aec74cf71f62601cd5842bc03a83261200802de365732a14b
languageName: node
linkType: hard

"@ethereumjs/rlp@npm:^4.0.1":
version: 4.0.1
resolution: "@ethereumjs/rlp@npm:4.0.1"
bin:
rlp: bin/rlp
checksum: 30db19c78faa2b6ff27275ab767646929207bb207f903f09eb3e4c273ce2738b45f3c82169ddacd67468b4f063d8d96035f2bf36f02b6b7e4d928eefe2e3ecbc
languageName: node
linkType: hard

"@ethereumjs/tx@npm:^4.1.2":
version: 4.2.0
resolution: "@ethereumjs/tx@npm:4.2.0"
dependencies:
"@ethereumjs/common": ^3.2.0
"@ethereumjs/rlp": ^4.0.1
"@ethereumjs/util": ^8.1.0
ethereum-cryptography: ^2.0.0
checksum: 87a3f5f2452cfbf6712f8847525a80c213210ed453c211c793c5df801fe35ecef28bae17fadd222fcbdd94277478a47e52d2b916a90a6b30cda21f1e0cdaee42
languageName: node
linkType: hard

"@ethereumjs/util@npm:^8.1.0":
version: 8.1.0
resolution: "@ethereumjs/util@npm:8.1.0"
dependencies:
"@ethereumjs/rlp": ^4.0.1
ethereum-cryptography: ^2.0.0
micro-ftch: ^0.3.1
checksum: 9ae5dee8f12b0faf81cd83f06a41560e79b0ba96a48262771d897a510ecae605eb6d84f687da001ab8ccffd50f612ae50f988ef76e6312c752897f462f3ac08d
languageName: node
linkType: hard

"@gar/promisify@npm:^1.1.3":
version: 1.1.3
resolution: "@gar/promisify@npm:1.1.3"
Expand Down Expand Up @@ -717,6 +759,7 @@ __metadata:
"@metamask/eslint-config-jest": ^11.1.0
"@metamask/eslint-config-nodejs": ^11.1.0
"@metamask/eslint-config-typescript": ^11.1.0
"@metamask/utils": ^8.1.0
"@types/cross-spawn": ^6.0.2
"@types/diff": ^5.0.0
"@types/jest": ^26.0.23
Expand Down Expand Up @@ -795,6 +838,43 @@ __metadata:
languageName: node
linkType: hard

"@metamask/utils@npm:^8.1.0":
version: 8.1.0
resolution: "@metamask/utils@npm:8.1.0"
dependencies:
"@ethereumjs/tx": ^4.1.2
"@noble/hashes": ^1.3.1
"@types/debug": ^4.1.7
debug: ^4.3.4
semver: ^7.5.4
superstruct: ^1.0.3
checksum: 4cbee36d0c227f3e528930e83f75a0c6b71b55b332c3e162f0e87f3dd86ae017d0b20405d76ea054ab99e4d924d3d9b8b896ed12a12aae57b090350e5a625999
languageName: node
linkType: hard

"@noble/curves@npm:1.1.0, @noble/curves@npm:~1.1.0":
version: 1.1.0
resolution: "@noble/curves@npm:1.1.0"
dependencies:
"@noble/hashes": 1.3.1
checksum: 2658cdd3f84f71079b4e3516c47559d22cf4b55c23ac8ee9d2b1f8e5b72916d9689e59820e0f9d9cb4a46a8423af5b56dc6bb7782405c88be06a015180508db5
languageName: node
linkType: hard

"@noble/hashes@npm:1.3.1":
version: 1.3.1
resolution: "@noble/hashes@npm:1.3.1"
checksum: 7fdefc0f7a0c1ec27acc6ff88841793e3f93ec4ce6b8a6a12bfc0dd70ae6b7c4c82fe305fdfeda1735d5ad4a9eebe761e6693b3d355689c559e91242f4bc95b1
languageName: node
linkType: hard

"@noble/hashes@npm:^1.3.1, @noble/hashes@npm:~1.3.0, @noble/hashes@npm:~1.3.1":
version: 1.3.2
resolution: "@noble/hashes@npm:1.3.2"
checksum: fe23536b436539d13f90e4b9be843cc63b1b17666a07634a2b1259dded6f490be3d050249e6af98076ea8f2ea0d56f578773c2197f2aa0eeaa5fba5bc18ba474
languageName: node
linkType: hard

"@nodelib/fs.scandir@npm:2.1.5":
version: 2.1.5
resolution: "@nodelib/fs.scandir@npm:2.1.5"
Expand Down Expand Up @@ -871,6 +951,34 @@ __metadata:
languageName: node
linkType: hard

"@scure/base@npm:~1.1.0":
version: 1.1.3
resolution: "@scure/base@npm:1.1.3"
checksum: 1606ab8a4db898cb3a1ada16c15437c3bce4e25854fadc8eb03ae93cbbbac1ed90655af4b0be3da37e12056fef11c0374499f69b9e658c9e5b7b3e06353c630c
languageName: node
linkType: hard

"@scure/bip32@npm:1.3.1":
version: 1.3.1
resolution: "@scure/bip32@npm:1.3.1"
dependencies:
"@noble/curves": ~1.1.0
"@noble/hashes": ~1.3.1
"@scure/base": ~1.1.0
checksum: 394d65f77a40651eba21a5096da0f4233c3b50d422864751d373fcf142eeedb94a1149f9ab1dbb078086dab2d0bc27e2b1afec8321bf22d4403c7df2fea5bfe2
languageName: node
linkType: hard

"@scure/bip39@npm:1.2.1":
version: 1.2.1
resolution: "@scure/bip39@npm:1.2.1"
dependencies:
"@noble/hashes": ~1.3.0
"@scure/base": ~1.1.0
checksum: c5bd6f1328fdbeae2dcdd891825b1610225310e5e62a4942714db51066866e4f7bef242c7b06a1b9dcc8043a4a13412cf5c5df76d3b10aa9e36b82e9b6e3eeaa
languageName: node
linkType: hard

"@sinonjs/commons@npm:^1.7.0":
version: 1.8.1
resolution: "@sinonjs/commons@npm:1.8.1"
Expand Down Expand Up @@ -953,6 +1061,15 @@ __metadata:
languageName: node
linkType: hard

"@types/debug@npm:^4.1.7":
version: 4.1.9
resolution: "@types/debug@npm:4.1.9"
dependencies:
"@types/ms": "*"
checksum: e88ee8b19d106f33eb0d3bc58bacff9702e98d821fd1ebd1de8942e6b97419e19a1ccf39370f1764a1dc66f79fd4619f3412e1be6eeb9f0b76412f5ffe4ead93
languageName: node
linkType: hard

"@types/diff@npm:^5.0.0":
version: 5.0.0
resolution: "@types/diff@npm:5.0.0"
Expand Down Expand Up @@ -1018,6 +1135,13 @@ __metadata:
languageName: node
linkType: hard

"@types/ms@npm:*":
version: 0.7.32
resolution: "@types/ms@npm:0.7.32"
checksum: 610744605c5924aa2657c8a62d307052af4f0e38e2aa015f154ef03391fabb4fd903f9c9baacb41f6e5798b8697e898463c351e5faf638738603ed29137b5254
languageName: node
linkType: hard

"@types/node@npm:*":
version: 20.1.1
resolution: "@types/node@npm:20.1.1"
Expand Down Expand Up @@ -2022,6 +2146,15 @@ __metadata:
languageName: node
linkType: hard

"crc-32@npm:^1.2.0":
version: 1.2.2
resolution: "crc-32@npm:1.2.2"
bin:
crc32: bin/crc32.njs
checksum: ad2d0ad0cbd465b75dcaeeff0600f8195b686816ab5f3ba4c6e052a07f728c3e70df2e3ca9fd3d4484dc4ba70586e161ca5a2334ec8bf5a41bf022a6103ff243
languageName: node
linkType: hard

"cross-spawn@npm:^6.0.0":
version: 6.0.5
resolution: "cross-spawn@npm:6.0.5"
Expand Down Expand Up @@ -2720,6 +2853,18 @@ __metadata:
languageName: node
linkType: hard

"ethereum-cryptography@npm:^2.0.0":
version: 2.1.2
resolution: "ethereum-cryptography@npm:2.1.2"
dependencies:
"@noble/curves": 1.1.0
"@noble/hashes": 1.3.1
"@scure/bip32": 1.3.1
"@scure/bip39": 1.2.1
checksum: 2e8f7b8cc90232ae838ab6a8167708e8362621404d26e79b5d9e762c7b53d699f7520aff358d9254de658fcd54d2d0af168ff909943259ed27dc4cef2736410c
languageName: node
linkType: hard

"exec-sh@npm:^0.3.2":
version: 0.3.4
resolution: "exec-sh@npm:0.3.4"
Expand Down Expand Up @@ -4838,6 +4983,13 @@ __metadata:
languageName: node
linkType: hard

"micro-ftch@npm:^0.3.1":
version: 0.3.1
resolution: "micro-ftch@npm:0.3.1"
checksum: 0e496547253a36e98a83fb00c628c53c3fb540fa5aaeaf718438873785afd193244988c09d219bb1802984ff227d04938d9571ef90fe82b48bd282262586aaff
languageName: node
linkType: hard

"micromatch@npm:^3.1.4":
version: 3.1.10
resolution: "micromatch@npm:3.1.10"
Expand Down Expand Up @@ -6069,6 +6221,17 @@ __metadata:
languageName: node
linkType: hard

"semver@npm:^7.5.4":
version: 7.5.4
resolution: "semver@npm:7.5.4"
dependencies:
lru-cache: ^6.0.0
bin:
semver: bin/semver.js
checksum: 12d8ad952fa353b0995bf180cdac205a4068b759a140e5d3c608317098b3575ac2f1e09182206bf2eb26120e1c0ed8fb92c48c592f6099680de56bb071423ca3
languageName: node
linkType: hard

"set-blocking@npm:^2.0.0, set-blocking@npm:~2.0.0":
version: 2.0.0
resolution: "set-blocking@npm:2.0.0"
Expand Down Expand Up @@ -6505,6 +6668,13 @@ __metadata:
languageName: node
linkType: hard

"superstruct@npm:^1.0.3":
version: 1.0.3
resolution: "superstruct@npm:1.0.3"
checksum: 761790bb111e6e21ddd608299c252f3be35df543263a7ebbc004e840d01fcf8046794c274bcb351bdf3eae4600f79d317d085cdbb19ca05803a4361840cc9bb1
languageName: node
linkType: hard

"supports-color@npm:^5.3.0":
version: 5.5.0
resolution: "supports-color@npm:5.5.0"
Expand Down

0 comments on commit 4804d3e

Please sign in to comment.