Skip to content
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

chore: make it build with strict null checks set to true #9554

Merged
merged 3 commits into from
Mar 19, 2025

Conversation

chriswk
Copy link
Member

@chriswk chriswk commented Mar 17, 2025

As part of preparation for ESM and node/TSC updates, this PR will make Unleash build with strictNullChecks set to true, since that's what's in our tsconfig file. Hence, this PR also removes the --strictNullChecks false flag in our compile tasks in package.json.

TL;DR - Clean up your code rather than turning off compiler security features :)

@chriswk chriswk self-assigned this Mar 17, 2025
Copy link

vercel bot commented Mar 17, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
unleash-docs ⬜️ Ignored (Inspect) Visit Preview Mar 19, 2025 7:58am
unleash-monorepo-frontend ⬜️ Ignored (Inspect) Visit Preview Mar 19, 2025 7:58am

Copy link
Contributor

@chriswk, core features have been modified in this pull request. Please review carefully!

Copy link
Contributor

github-actions bot commented Mar 17, 2025

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

PackageVersionScoreDetails
npm/typescript 5.8.2 🟢 8.7
Details
CheckScoreReason
Packaging⚠️ -1packaging workflow not detected
Code-Review🟢 10all changesets reviewed
Dependency-Update-Tool🟢 10update tool detected
Maintained🟢 1030 commit(s) and 12 issue activity found in the last 90 days -- score normalized to 10
Security-Policy🟢 10security policy file detected
Dangerous-Workflow🟢 10no dangerous workflow patterns detected
Token-Permissions🟢 10GitHub workflow tokens follow principle of least privilege
License🟢 10license file detected
CII-Best-Practices⚠️ 0no effort to earn an OpenSSF best practices badge detected
Signed-Releases⚠️ 0Project has not signed or included provenance with any releases.
Binary-Artifacts🟢 10no binaries found in the repo
Vulnerabilities🟢 100 existing vulnerabilities detected
Branch-Protection⚠️ -1internal error: error during branchesHandler.setup: internal error: githubv4.Query: Resource not accessible by integration
SAST🟢 10SAST tool is run on all commits
Pinned-Dependencies🟢 6dependency not pinned by hash detected -- score normalized to 6
Fuzzing🟢 10project is fuzzed
CI-Tests🟢 1030 out of 30 merged PRs checked by a CI test -- score normalized to 10
Contributors🟢 10project has 36 contributing companies or organizations
npm/typescript 5.8.2 🟢 8.7
Details
CheckScoreReason
Packaging⚠️ -1packaging workflow not detected
Code-Review🟢 10all changesets reviewed
Dependency-Update-Tool🟢 10update tool detected
Maintained🟢 1030 commit(s) and 12 issue activity found in the last 90 days -- score normalized to 10
Security-Policy🟢 10security policy file detected
Dangerous-Workflow🟢 10no dangerous workflow patterns detected
Token-Permissions🟢 10GitHub workflow tokens follow principle of least privilege
License🟢 10license file detected
CII-Best-Practices⚠️ 0no effort to earn an OpenSSF best practices badge detected
Signed-Releases⚠️ 0Project has not signed or included provenance with any releases.
Binary-Artifacts🟢 10no binaries found in the repo
Vulnerabilities🟢 100 existing vulnerabilities detected
Branch-Protection⚠️ -1internal error: error during branchesHandler.setup: internal error: githubv4.Query: Resource not accessible by integration
SAST🟢 10SAST tool is run on all commits
Pinned-Dependencies🟢 6dependency not pinned by hash detected -- score normalized to 6
Fuzzing🟢 10project is fuzzed
CI-Tests🟢 1030 out of 30 merged PRs checked by a CI test -- score normalized to 10
Contributors🟢 10project has 36 contributing companies or organizations

Scanned Files

  • package.json
  • yarn.lock

Comment on lines -1 to -2
const joi = require('joi');
const { nameType } = require('../routes/util');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be. All require is also gone now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we still have one in version.ts

@chriswk chriswk force-pushed the fix/strictNullChecks branch from 81aeb10 to 75778dd Compare March 18, 2025 19:15
@chriswk chriswk marked this pull request as ready for review March 18, 2025 19:33
Copy link
Contributor

@gastonfournier gastonfournier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can identify a few places where the syntax could be better, but weighing in the benefits of this PR vs being nitpicking on details, I'm inclined to ✔️

@@ -27,6 +27,7 @@ export default class RemoteAddressStrategy extends Strategy {
return false;
}
}
return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we didn't have a default response here, I guess some treats void as false.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better here would probably have been to drop the isValid check and just try to construct the Address4's so we'd always hit the catch part of the try/catch

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

absolutely! Just highlighting a great catch

const { version } = module.exports;
export default version;
module.exports = version;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

@chriswk chriswk merged commit efcf044 into main Mar 19, 2025
12 of 14 checks passed
@chriswk chriswk deleted the fix/strictNullChecks branch March 19, 2025 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants