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: add debug logger for fuel-ts ecosystem #2366

Merged
merged 26 commits into from
Jun 20, 2024

Conversation

maschad
Copy link
Member

@maschad maschad commented May 22, 2024

A simple logger for our packages. This is particularly useful when debugging BNs, Hexs, Addresses and other types that are common in transactions and bytecode. Below is an example of a formatted BN when debugging the calculateTXFeeForSummary function.

Screenshot 2024-06-12 at 6 20 15 PM

Closes #2298

@maschad maschad changed the title chore: added initial logger chore: add debug logger for fuel ecosystem May 23, 2024
@maschad maschad added the chore Issue is a chore label May 26, 2024
@maschad maschad self-assigned this May 26, 2024
@maschad maschad changed the title chore: add debug logger for fuel ecosystem chore!: add debug logger for fuel ecosystem May 26, 2024
@maschad maschad changed the title chore!: add debug logger for fuel ecosystem chore!: add debug logger for fuel-ts ecosystem May 26, 2024
@maschad maschad force-pushed the mc/chore/add-internal-logger branch from 823f8fb to d7fc2a4 Compare May 26, 2024 22:45
package.json Outdated Show resolved Hide resolved
@arboleya arboleya added this to the 0.x mainnet milestone Jun 12, 2024
@maschad maschad marked this pull request as ready for review June 12, 2024 22:25
@maschad maschad requested a review from arboleya June 12, 2024 22:25
@maschad maschad changed the title chore: add debug logger for fuel-ts ecosystem chore!: add debug logger for fuel-ts ecosystem Jun 17, 2024
@maschad
Copy link
Member Author

maschad commented Jun 18, 2024

Would a loggers folder with all available loggers also be valuable?

/loggers
    /prefixLogger
    /defaultLogger
    /walletLogger

Maybe if we decide to add more complex loggers but given these are just singular functions having a folder just for a function seems a bit overkill.

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.

Looks good to me 🍏 🚀

@maschad maschad enabled auto-merge (squash) June 20, 2024 11:53
arboleya

This comment was marked as duplicate.

@maschad maschad requested a review from arboleya June 20, 2024 15:27
Copy link
Contributor

Coverage Report:

Lines Branches Functions Statements
80.98%(+0.02%) 72.52%(+0.1%) 77.98%(-0.32%) 81.04%(+0%)
Changed Files:
Ok File (✨=New File) Lines Branches Functions Statements
🔴 ✨ packages/logger/src/index.ts 83.78%
(+83.78%)
80%
(+80%)
52.38%
(+52.38%)
78.57%
(+78.57%)
✨ packages/logger/src/utils.ts 100%
(+100%)
100%
(+100%)
100%
(+100%)
100%
(+100%)

@maschad maschad merged commit de4e16f into master Jun 20, 2024
20 checks passed
@maschad maschad deleted the mc/chore/add-internal-logger branch June 20, 2024 18:40
@arboleya
Copy link
Member

@maschad Should this have been a breaking change?

If yes, please describe it in the PR description.

@maschad maschad changed the title chore!: add debug logger for fuel-ts ecosystem chore: add debug logger for fuel-ts ecosystem Jun 24, 2024
@maschad
Copy link
Member Author

maschad commented Jun 24, 2024

@arboleya the validate changesets workflow treated this as a breaking change in our current release process (see #2366 (comment))

Upon further discussion in the 24-06-2024 Sync I have updated the PR title on the recommendation from @nedsalk so that the release PR has not mentioned it as breaking.

@arboleya
Copy link
Member

@maschad You changed the title after the merge, and although it looks right in the changeset PR (not a breaking), you can see that it is still considered a minor bump because the changeset remains the same - with the !. IIRC, you'd need to open up a new PR modifying the existing changeset. Maybe @nedsalk can offer better guidance here.

Finally, I think it should still be a feat (for a new feature), but a patch for changeset purposes:

---
"@fuel-ts/logger": patch
---

feat: add debug logger for fuel-ts ecosystem

@nedsalk
Copy link
Contributor

nedsalk commented Jun 26, 2024

@arboleya you're right, the changeset should be changed manually, there's no workaround for that. sigh I can't wait for the day when a singular fuels package becomes a reality.

nedsalk pushed a commit that referenced this pull request Jun 27, 2024
* chore: added initial logger

* docs: add changeset

* fix: added enginge to package.json

* docs: updated changeset

* test: add more tests

* build: undo coverage changes

* feat: fix build issues

* feat: update BN formatter

* build: update lockfile

* test: add more tests

* docs: update changeset

* test: add test environment config

* test: remove browser testing

* docs: revert changeset

* test: add formatter tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Issue is a chore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a internal logger for debugging
6 participants