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

Consolidate path to string map and parameter type #576

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

Conversation

aaronccasanova
Copy link

@aaronccasanova aaronccasanova commented Feb 5, 2022

Hello 👋

While familiarizing myself with the library, I noticed some small opportunities for consolidation:

  • Use built-in PropertyKey type instead of string | number | symbol union
  • Use .map(String) instead of .map((p) => p.toString())

These updates have no meaningful impact on the behavior or functionality of the library. Therefore, feel free to close this PR if the suggestions are deemed unnecessary 🙂

@benjamind
Copy link
Collaborator

While the PropertyKey type change I agree with, I think the other change of using String Constructor for the map argument would be a significant performance cost. I think the constructor is at least an order of magnitude slower than the toString implementation. If you'd like to cleanup the PR to just the PropertyKey change I would be happy to merge though!

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.

2 participants