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(entity): remove ComparerStr #2584

Merged
merged 1 commit into from
Jun 18, 2020
Merged

fix(entity): remove ComparerStr #2584

merged 1 commit into from
Jun 18, 2020

Conversation

LMFinney
Copy link
Contributor

The compare function is used in two places, neither of which expect it to be able to return a string:
The first caller is the Array prototype sort function, and there it "should return a negative, zero, or positive value, depending on the arguments".
The second caller does a numerical comparison with the result.

Even though an id can be a string, the result of a comparison shouldn't be.

This might be a breaking change.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

According to the typing, a sortComparer can return a string, but it would be a mistake to allow that. Even though an id can be a string, the result of a comparison shouldn't be.

Closes #

What is the new behavior?

Remove the ComparerStr option for Comparar.

Does this PR introduce a breaking change?

[x ] Yes
[ ] No

Functions that return a string instead of a number would not compile. The user would need to fix the comparer.

Other information

@ngrxbot
Copy link
Collaborator

ngrxbot commented Jun 16, 2020

Preview docs changes for 96352e0 at https://previews.ngrx.io/pr2584-96352e08/

@LMFinney
Copy link
Contributor Author

I can't get build-affected to fail locally, and I can't imagine how my change causes the compilation error that's shown. Could there be another issue?

@brandonroberts
Copy link
Member

Yea, there is a separate issue. Store should have been built before Entity with the affected commands. I can reproduce it locally by deleting the @ngrx packages under node_modules before running nx affected --target=build-release with your branch.

@brandonroberts
Copy link
Member

Will you squash down to a single commit, and add the breaking change to the commit message?

@LMFinney
Copy link
Contributor Author

I squashed the commits and tweaked the commit message, as requested.

The compare function is used in two places, neither of which expect it to be able to return a string:
The first caller (https://github.com/ngrx/platform/blob/master/modules/entity/src/sorted_state_adapter.ts#L174) is the Array prototype sort function, and there it "should return a negative, zero, or positive value, depending on the arguments" (https://www.w3schools.com/js/js_array_sort.asp).
The second caller (https://github.com/ngrx/platform/blob/master/modules/entity/src/sorted_state_adapter.ts#L187) does a numerical comparison with the result.

Even though an id can be a string, the result of a comparison shouldn't be.

This will be a breaking change; any client that returned a string from their comparer will have a compilation failure. Note that any implementation that would not compile because of this would not have been working correctly in the first place.
@brandonroberts brandonroberts merged commit 4796c97 into ngrx:master Jun 18, 2020
BioPhoton pushed a commit to BioPhoton/platform that referenced this pull request Jun 23, 2020
BREAKING CHANGE:

The compare function is used in two places, neither of which expect it to be able to return a string:
The first caller is the Array prototype sort function, and there it "should return a negative, zero, or positive value, depending on the arguments".
The second caller does a numerical comparison with the result.

Even though an id can be a string, the result of a comparison shouldn't be.

BEFORE:

The sortComparer types allow for a string to be returned

AFTER:

The sortComparer types only allow a number to be returned
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