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.

Sign up for GitHub

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

Jump to bottom

chore: move type:check to lint hook #3381

Merged
merged 21 commits into from
Nov 20, 2024
Merged

chore: move type:check to lint hook #3381

merged 21 commits into from
Nov 20, 2024

Conversation

Rohit-Bhetal
Copy link
Contributor

@Rohit-Bhetal Rohit-Bhetal commented Nov 11, 2024 •
edited
Loading

Summary

This PR moves the TypeScript type checking from the various build-related scripts (pretest, post-build, etc.) to a dedicated lint script. This helps improve the development speed by reducing the number of redundant type checks performed during the build process.

Checklist

  • All changes are covered by tests (or not applicable)
  • All changes are documented (or not applicable)
  • I reviewed the entire PR myself (preferably, on GH UI)
  • I described all Breaking Changes (or there's none)

@Rohit-Bhetal
refactor: move type:check to lint hook
62b5607 
- Removed type:check from pretest, build, and postbuild scripts
- Added type checking to lint command
- Improves development speed by reducing redundant type checks

Fixes FuelLabs#3248
@vercel Vercel
Copy link

vercel bot commented Nov 11, 2024

@Rohit-Bhetal is attempting to deploy a commit to the Fuel Labs Team on Vercel.

A member of the Team first needs to authorize it.

@CLAassistant
Copy link

CLAassistant commented Nov 11, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@vercel Vercel
Copy link

vercel bot commented Nov 11, 2024 •
edited
Loading

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

Name Status Preview Comments Updated (UTC)
fuels-template ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 20, 2024 2:00pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
ts-docs-api ⬜️ Ignored (Inspect) Nov 20, 2024 2:00pm

@Rohit-Bhetal
refactor: move type:check to lint hook- Removed type:check from prete…
4d67d8f 
…st, build, and postbuild scripts- Added type checking to lint command- Improves development speed by reducing redundant type checksFixes FuelLabs#3248
Rohit-Bhetal
Rohit-Bhetal commented Nov 11, 2024
View reviewed changes
Copy link
Contributor Author

@Rohit-Bhetal Rohit-Bhetal left a comment

Choose a reason for hiding this comment

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

Updated other package.json file also

@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Nov 18, 2024 •
edited
Loading

CodSpeed Performance Report

Merging #3381 will not alter performance

Comparing Rohit-Bhetal:fix/move-type-check-to-lint (01020a5) with master (7f92490)

Summary

✅ 18 untouched benchmarks

@Torres-ssf Torres-ssf added chore Issue is a chore good first issue Suitable for newcomers looking to contribute labels Nov 18, 2024
maschad
maschad previously approved these changes Nov 18, 2024
View reviewed changes
Copy link
Member

@maschad maschad left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @Rohit-Bhetal 👍🏾

@Torres-ssf Torres-ssf changed the title refactor: move type:check to lint hook chore: move type:check to lint hook Nov 18, 2024
Dhaiwat10
Dhaiwat10 previously approved these changes Nov 19, 2024
View reviewed changes
petertonysmith94
petertonysmith94 previously requested changes Nov 19, 2024
View reviewed changes
Copy link
Contributor

@petertonysmith94 petertonysmith94 left a comment

Choose a reason for hiding this comment

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

I believe we need to run turbo run lint from the root level package.json.

Maybe we could keep the type:check command hook for the project level package.json, and add the following on the root level:

...,
"lint": "run-s type:check-tests lint:check prettier:check type:check",
"type:check": "turbo run type:check",
...,

@maschad
Copy link
Member

maschad commented Nov 19, 2024

I believe we need to run turbo run lint from the root level package.json.

Maybe we could keep the type:check command hook for the project level package.json, and add the following on the root level:

...,
"lint": "run-s type:check-tests lint:check prettier:check type:check",
"type:check": "turbo run type:check",
...,

Right good catch @petertonysmith94 , if we are going that route then we will also have to add type:check to the turbo tasks

@maschad maschad dismissed their stale review November 19, 2024 10:31

Dismissing based on above comment

Rohit-Bhetal
Rohit-Bhetal commented Nov 19, 2024
View reviewed changes
Copy link
Contributor Author

@Rohit-Bhetal Rohit-Bhetal left a comment

Choose a reason for hiding this comment

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

Added a type:check to turbo tasks and update root package.json lint script and made task to run the type check in turbo.json

@Rohit-Bhetal
Copy link
Contributor Author

This PR needs a changeset, please run pnpm changeset from the root directory and commit the changeset.

yep done

petertonysmith94
petertonysmith94 reviewed Nov 20, 2024
View reviewed changes
packages/create-fuels/package.json Outdated Show resolved Hide resolved
packages/fuel-gauge/package.json Outdated Show resolved Hide resolved
packages/fuels/package.json Outdated Show resolved Hide resolved
internal/benchmarks/package.json Outdated Show resolved Hide resolved
apps/docs-snippets/package.json Outdated Show resolved Hide resolved
.changeset/polite-seas-taste.md Outdated Show resolved Hide resolved
.changeset/polite-seas-taste.md Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
Rohit-Bhetal and others added 8 commits November 20, 2024 13:57
@petertonysmith94 petertonysmith94 dismissed stale reviews from danielbate and themself November 20, 2024 08:56

Changeset added

@petertonysmith94 petertonysmith94 enabled auto-merge (squash) November 20, 2024 14:21
@petertonysmith94 petertonysmith94 merged commit 30585c1 into FuelLabs:master Nov 20, 2024
23 of 25 checks passed
@maschad maschad mentioned this pull request Nov 21, 2024
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petertonysmith94 petertonysmith94 petertonysmith94 approved these changes

@maschad maschad maschad approved these changes

@danielbate danielbate danielbate approved these changes

@digorithm digorithm Awaiting requested review from digorithm digorithm is a code owner

@arboleya arboleya Awaiting requested review from arboleya arboleya is a code owner

@Torres-ssf Torres-ssf Awaiting requested review from Torres-ssf Torres-ssf is a code owner

@nedsalk nedsalk Awaiting requested review from nedsalk nedsalk is a code owner

@Dhaiwat10 Dhaiwat10 Awaiting requested review from Dhaiwat10 Dhaiwat10 is a code owner

Assignees

@Rohit-Bhetal Rohit-Bhetal

Labels
chore Issue is a chore good first issue Suitable for newcomers looking to contribute
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Altertype:check command
7 participants
@Rohit-Bhetal @CLAassistant @maschad @danielbate @petertonysmith94 @Dhaiwat10 @Torres-ssf