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

feat: noImplicitAny #2379

Merged
merged 8 commits into from
Aug 7, 2024
Merged

feat: noImplicitAny #2379

merged 8 commits into from
Aug 7, 2024

Conversation

pedromtec
Copy link
Contributor

@pedromtec pedromtec commented Jul 11, 2024

What's the purpose of this pull request?

Strict mode in TypeScript is a way to opt into a more strict type-checking behavior to catch more potential issues at compile time. When enabled, it activates a set of stricter type-checking options that enforce more constraints on your code. Here's how it works and what it does:

No Implicit Any (noImplicitAny): This prevents variables and parameters from defaulting to the any type when TypeScript is unable to infer a more specific type. This helps catch cases where you might have forgotten to specify a type or TypeScript can't infer one for you.

To enable strict mode, you can set "strict": true in your tsconfig.json file. This is equivalent to enabling all of the above options individually.

The idea of ​​this initiative is to enable rule by rule incrementally so that it is possible to open smaller PRs with low risk.

Task: https://vtex-dev.atlassian.net/browse/SFS-1124

How it works?

The first PR enables noImplicitAny rule. So codes like that will not work:

What's the type of address?

function printAddresss(address) {
    console.log(address) 
}

How to test it?

  • The pipeline (build, tests, and lint) should work for @faststore packages and their dependencies (current branch).

  • Install the version of this PR in the starter.store and test if the build, tests and override feature works.

"@faststore/core": "https://pkg.csb.dev/vtex/faststore/commit/1b7aa312/@faststore/core"
"@faststore/cli": "https://pkg.csb.dev/vtex/faststore/commit/1b7aa312/@faststore/cli",

I welcome other suggestions and help with testing 🥇

Starters Deploy Preview

References

Copy link

vercel bot commented Jul 11, 2024

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

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
faststore-site ⬜️ Ignored (Inspect) Visit Preview Aug 6, 2024 1:40pm

Copy link

codesandbox-ci bot commented Jul 11, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

packages/core/src/sdk/overrides/overrides.ts Outdated Show resolved Hide resolved
@@ -32,14 +37,14 @@ function SearchDropdown({ sort, ...otherProps }) {
linkProps={{
href: formatSearchPath({
term: suggestion,
sort: sort as SearchState['sort'],
sort: sort,
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion:

Suggested change
sort: sort,
sort,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

suggestion:

💯

}),
onClick: () =>
onSearchSelection?.(
suggestion,
formatSearchPath({
term: suggestion,
sort: sort as SearchState['sort'],
sort: sort,
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion:

Suggested change
sort: sort,
sort,

/**
* Modify TypeScript compilation settings (tsconfig.json) to disable specific strict
* type checking rules when files are moved to the .faststore folder.
* The idea is to change the strict to false when all strict rules are migrated.
Copy link
Contributor

Choose a reason for hiding this comment

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

If I got it right, we will remove this part when all rules are migrated, right?
suggestion: maybe we can add a todo markup here - // TODO:

Copy link
Contributor

@hellofanny hellofanny left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@pedromtec
Copy link
Contributor Author

Team (small change). I changed from ts-expect-error to ts-ignore because ts-expect-error generates an error when the line has no error. In other words, it always expects the line to have an error. However, when I disable noImplictyAny, the lines stop having errors within the .faststore.

@pedromtec pedromtec merged commit 833e407 into main Aug 7, 2024
7 checks passed
@pedromtec pedromtec deleted the feat/noImplicitAny branch August 7, 2024 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants