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

fix(adapter-commons): faster sorter #3495

Merged
merged 1 commit into from
Jun 9, 2024
Merged

fix(adapter-commons): faster sorter #3495

merged 1 commit into from
Jun 9, 2024

Conversation

fratzinger
Copy link
Member

This is a small refactoring.

  • check for 'null && undefined' in one if
  • detect typeof only once
  • move get fn outside
  • check key.includes('.') before key.split('.')
  • type compareNSB & compareString

- check for 'null && undefined' in one if
- detect typeof only once
- move get fn outside
- check key.includes('.') before key.split('.')
- type compareNSB & compareString
Copy link

Deploying feathers-dove with  Cloudflare Pages  Cloudflare Pages

Latest commit: a30281f
Status: ✅  Deploy successful!
Preview URL: https://1d8832bb.feathers.pages.dev
Branch Preview URL: https://refactor-sorter.feathers.pages.dev

View logs

@daffl daffl changed the title refactor(adapter-commons): faster sorter fix(adapter-commons): faster sorter Jun 9, 2024
@daffl
Copy link
Member

daffl commented Jun 9, 2024

This looks good, thank you. I wonder if there is a more generic package for this kind of thing. Doesn't seem to be something we should have to maintain ourselves - I took this from NeDB when it stopped being maintained.

@daffl daffl merged commit 22243e4 into dove Jun 9, 2024
5 checks passed
@daffl daffl deleted the refactor/sorter branch June 9, 2024 20:20
@fratzinger
Copy link
Member Author

https://github.com/kofrasa/mingo could help. But I think it's fine. It's a small utility which is not a subject of change often and most adapters use @feathersjs/adapter-commons for other reasons anyways. It's good to have it in this package.

I think we should not overcomplicate this. Or we end up importing and exporting from another library and their versioning has to align with our versioning. We have enough of these problems.

@daffl
Copy link
Member

daffl commented Jun 9, 2024

True. It might be worth getting more test coverage for it eventually if we do want to keep it around.

@fratzinger
Copy link
Member Author

💯. I also thought about that.

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