Skip to content

Conversation

@marcohefti
Copy link
Contributor

…tree shaking (fixes #1015)

This change replaces the default export in the utils package with named exports, which allows for better tree shaking and smaller build sizes in user code applications. It also enforces the no-default-export ESLint rule to ensure consistent code style

…tree shaking (fixes RequestNetwork#1015)

This change replaces the default export in the utils package with named exports, which allows for better tree shaking and smaller build sizes in user code applications. It also enforces the no-default-export ESLint rule to ensure consistent code style
@MantisClone MantisClone changed the title Replace default export with named exports in utils package to enable … chore(utils): replace default export with named exports Dec 22, 2022
Copy link
Member

@MantisClone MantisClone left a comment

Choose a reason for hiding this comment

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

Hey @roadrunner21. Thanks for this PR submission! I reviewed it and it looks good, but the build is failing due to prettier failure. Would you mind installing husky and prettier using yarn install so that the pre-commit checks are enabled and then adding an empty commit with git commit --allow-empty to run them.

Copy link
Contributor

@alexandre-abrioux alexandre-abrioux left a comment

Choose a reason for hiding this comment

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

Thank you for this 🙂

@MantisClone MantisClone self-requested a review December 22, 2022 19:39
Copy link
Contributor

@benjlevesque benjlevesque left a comment

Choose a reason for hiding this comment

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

this LGTM, but I only see updates on the utils package. There are many usage of that package in the monorepo, they need to be updated too.

marcohefti and others added 3 commits December 25, 2022 14:29
This commit applies the changes made to the utils package to the rest of the monorepo. The changes include replacing the default export with named exports and enforcing the no-default-export ESLint rule.

NOTE: Not all changes could be tested (RequestNetwork#1029, RequestNetwork#1030, RequestNetwork#1031)
@MantisClone
Copy link
Member

MantisClone commented Jan 3, 2023

@roadrunner21

I believe the payment-detection tests are failing due to a circular dependency issue. Are you able to fix this?

I'm surprised that the smart-contracts tests are failing but I think that #1038 will fix it.

Copy link
Member

@MantisClone MantisClone left a comment

Choose a reason for hiding this comment

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

@roadrunner21 some of your commits are not signed.
image

Here's how I recommend you fix it:

  1. Configure git to use your SSH signing key
    $ git config --global gpg.format ssh
    $ git config --global user.signingkey ~/.ssh/id_ed25519.pub
    
  2. Add your SSH signing key to Github
    • You can reuse your SSH “Authentication” Key as a “Signing” Key but it must be added separately.
    • Example: image
  3. Configure git to sign all commits
    git config --global commit.gpgsign true
    
  4. Fix un-signed commits using rebase
    git rebase --exec 'git commit --amend --no-edit -n -S' -i master
    

leoslr and others added 8 commits January 3, 2023 17:15
This commit applies the changes made to the utils package to the rest of the monorepo. The changes include replacing the default export with named exports and enforcing the no-default-export ESLint rule.

NOTE: Not all changes could be tested (RequestNetwork#1029, RequestNetwork#1030, RequestNetwork#1031)
…aki' into econ/dw-17/utils-package-tree-shaki
# Conflicts:
#	packages/smart-contracts/test/contracts/ChainlinkConversionPath.test.ts
Copy link
Member

@MantisClone MantisClone left a comment

Choose a reason for hiding this comment

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

Hello @roadrunner21 👋 Thank you for your latest changes!

Overview

I discussed these changes with @alexandre-abrioux and @benjlevesque. We concluded that many of the functions in utils need to be renamed to be more self-descriptive due to this refactor.

Details

In the past, module names like identity or amount helped clarify what a function was doing. For example: identity.areEqual() checks if two identities are equal and amount.reduce() reduces an amount.

After refactoring, these functions can be called without their module name areEqual() and reduce(). This is good because the caller need only import the functions that are used and not the entire module. But it makes the code harder to read.

Thus, we'd like to see the following change:

  • Please rename ambiguous functions. Examples:

    • areEqual() from the identity module --> areEqualIdentities()
    • reduce() from the amount module --> reduceAmount()
    • min() from the bignumber model --> minBigNumber()
    • recover() from the signature module --> recoverSigner()
  • But please leave sufficiently descriptive functions unchanged. Examples:

    • normalizeIdentityValue() from the identity module
    • encrypt() from the encryption module
    • retry() from the retry module
    • all the functions in the provider module (some don't include the word "provider", but I think they're all self-descriptive enough)

If you have any questions, please don't hesitate to ask!

As discussed with @alexandre-abrioux and @benjlevesque, many of the functions in the utils module have been refactored and need to be renamed for improved code readability.

Signed-off-by: marcohefti <marco@heftiweb.ch>
@marcohefti marcohefti removed the request for review from benjlevesque January 10, 2023 15:04
Copy link
Member

@MantisClone MantisClone left a comment

Choose a reason for hiding this comment

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

Hello, @roadrunner21 Sorry for the delay. I read through every line and left a few more change requests.

I'm excited. Your changes will cut down the number of unnecessary imports across the code base!

-Removed list of utils from README to avoid staleness
-Changed 'hasError' to 'identityHasError'
-Changed 'sign' back to original name 'sign'
-Changed 'recover' to 'recoverSigner'

Addressed comments by @MantisClone

Signed-off-by: marcohefti <marco@heftiweb.ch>
-Removed the named const from crypto-wrapper.ts and exported functions individually
-Removed the named const from ec-utils.ts and exported functions individually
-Renamed recover() to recoverSigner() in ec-utils.ts
-Prefixed functions with 'ec' to prevent duplicate variables
-Reverted normalizeData() to normalize()

Signed-off-by: marcohefti <marco@heftiweb.ch>
Copy link
Member

@MantisClone MantisClone 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 bearing with me @roadrunner21 this is great work.

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.

Utils package tree shaking

5 participants