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

refactor: extract shared utility functions #550

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Phillip9587
Copy link
Contributor

This PR refactors the code by moving the getCharset and typeChecker functions, which were previously duplicated across different parts of the codebase, into a shared lib/utils.js file.

Key Benefits:

  • Code reuse: Both functions are now centralized, reducing duplication.
  • Size optimization: By eliminating repeated code, this change helps shave off some bytes.
  • Maintainability: Future updates to these functions can be made in one place, simplifying maintenance.

No functionality has been altered—this is strictly a refactor for improved efficiency and modularity.

@Phillip9587
Copy link
Contributor Author

I'm unsure if internal refactorings need a history entry? What is your policy on this @UlisesGascon?

Copy link
Member

@UlisesGascon UlisesGascon left a comment

Choose a reason for hiding this comment

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

LGTM!

@UlisesGascon
Copy link
Member

What is your policy on this @UlisesGascon?

I think that making a reference is fine, so far if the change does not affect dependencies or API... it might be ignored as well. IMO, The aim for the HISTORY.md is to provide an easy way for the users to understand what has changed on how that might affect them. So in this case I am very neutral 👍

BTW... there is a discussion on going about the HISTORY.md, feel free to participate and provide your feedback expressjs/discussions#293

Also as you are doing a lot of contributions you might want to join us in Slack #express channel in the OpenJS foundation slack (invitation)

@Phillip9587
Copy link
Contributor Author

I think that making a reference is fine, so far if the change does not affect dependencies or API... it might be ignored as well. IMO, The aim for the HISTORY.md is to provide an easy way for the users to understand what has changed on how that might affect them. So in this case I am very neutral 👍

Thanks for the clarification.

Also as you are doing a lot of contributions you might want to join us in Slack #express channel in the OpenJS foundation slack (invitation)

Thanks for the invite. I'm happy to help.

@wesleytodd
Copy link
Member

Same question as in #551. We should align on if this or @ctcpip's revival of #544 is better.

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.

3 participants