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

Ability to specify whether null records should be first or last when sorting. #296

Merged
merged 1 commit into from
Aug 20, 2022

Conversation

Elbarae1921
Copy link
Contributor

@Elbarae1921 Elbarae1921 commented Aug 20, 2022

Closes #267.

I believe this is the official "typeorm" way of doing it (typeorm/typeorm#685). I think it doesn't work on all databases but that's for the typeorm project to worry about.

query.addOrderBy('record.field', 'ASC', 'NULLS LAST');

I added a new test closure for the new functionality, LMK if maybe there should be more, I also updated the readme.

… beginning or the end of the result set when sorting
@codecov
Copy link

codecov bot commented Aug 20, 2022

Codecov Report

Merging #296 (5abdeed) into master (f7d0eb4) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #296      +/-   ##
==========================================
+ Coverage   95.74%   95.76%   +0.01%     
==========================================
  Files           5        5              
  Lines         235      236       +1     
  Branches       79       80       +1     
==========================================
+ Hits          225      226       +1     
  Misses          9        9              
  Partials        1        1              
Impacted Files Coverage Δ
src/paginate.ts 96.47% <100.00%> (+0.02%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Elbarae1921
Copy link
Contributor Author

This is what the config looks like now

return paginate(query, dataRepository, {
      sortableColumns: ['id', 'name', 'color', 'age'],
      nullSort: 'last', // <--- new field
      searchableColumns: ['name', 'color', 'age'],
      defaultSortBy: [['id', 'DESC']],
      filterableColumns: {
        age: [FilterOperator.GTE, FilterOperator.LTE],
      },
});

Initially I thought it would be useful to have it in the query instead, like:

&sortBy=color:DESC:NULLS LAST

But it seems more fitting for it to be a config, as it's not something I imagine you'd change often.
Let me know what you think.

@Elbarae1921 Elbarae1921 mentioned this pull request Aug 20, 2022
Copy link
Owner

@ppetzold ppetzold left a comment

Choose a reason for hiding this comment

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

Agree with everything.

LGTM 🌟

@ppetzold ppetzold merged commit 081177a into ppetzold:master Aug 20, 2022
@ppetzold
Copy link
Owner

🎉 This PR is included in version 4.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ppetzold
Copy link
Owner

@Elbarae1921 Ah sorry, just looking at it after merging:

  • readme says default is last, code defaults to first tho
  • what was the default before this PR?
  • can you add a test with first as well?

@Elbarae1921
Copy link
Contributor Author

@ppetzold my bad. Initially, I set last as default but since the default behavior before was first, I changed it but forgot to change the readme after.

Sure, I can. Would have to open a new PR tho right?

@ppetzold
Copy link
Owner

@ppetzold my bad. Initially, I set last as default but since the default behavior before was first, I changed it but forgot to change the readme after.

Sure, I can. Would have to open a new PR tho right?

Yes pls :)

@a-soppe-emit
Copy link

#296 (comment) I believe this is the official "typeorm" way of doing it (typeorm/typeorm#685). I think it doesn't work on all databases but that's for the typeorm project to worry about.

You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'NULLS FIRST LIMIT 20' at line 1

Version 4.2.0 sadly breaks pagination when using MySQL. Invalid syntax NULLS FIRST/LAST is not stripped out by TypeORM for databases like MySQL.

Reverted back to 4.1.0 for now.

@ppetzold
Copy link
Owner

ppetzold commented Aug 22, 2022

#296 (comment) I believe this is the official "typeorm" way of doing it (typeorm/typeorm#685). I think it doesn't work on all databases but that's for the typeorm project to worry about.

You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'NULLS FIRST LIMIT 20' at line 1

Version 4.2.0 sadly breaks pagination when using MySQL. Invalid syntax NULLS FIRST/LAST is not stripped out by TypeORM for databases like MySQL.

Reverted back to 4.1.0 for now.

@Elbarae1921 Can you remove the default then / only append to query if set? And add in comment/README that not all database types are supported?

FrancYescO pushed a commit to xMase/nestjs-paginate that referenced this pull request Jan 2, 2023
… beginning or the end of the result set when sorting (ppetzold#296)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sorting by NULL LAST
3 participants