refactor(platform-browser): remove obsolete shim for Map comparison in Jasmine #45521
Conversation
@toastwaffle thanks for the cleanup! I've looked at the PR where the changes were introduced (see #22983) and I think we should also do a cleanup of the packages/types.d.ts and probably the packages/core/test/testing_internal_spec.ts as well.
AndrewKushnir
added
action: review
target: patch
AndrewKushnir
added
the
refactoring
|
@toastwaffle could you please also update the commit message to make it start with |
toastwaffle
changed the title
|
I've removed one type alias from |
AndrewKushnir
added
action: presubmit
|
@toastwaffle thanks for addressing the feedback. It looks like there are some legit failures in a global presubmit (see here). Could you please investigate and let us know if this may end up resulting in a breaking change? I think it'd still make sense to land this change, but we'd need to include extra info into a commit message and only land in a major version of Angular (i.e. v14.0.0). |
|
@AndrewKushnir those test failures are valid; I have fixes for them in this CL (sponge). Are we able to patch this PR with those fixes, or do we still need to wait for a major version (when is v14.0.0 due to land?) |
|
@toastwaffle thanks for the fixes! I've looked at the fixes and I'm wondering if they can be landed now without changes from this PR (if the tests would still be "green")? If yes, that'd be the best option and once the changes from your CL land, we can land this PR. We had a discussion about these changes with the team and the custom Jasmine matchers should not be available to users outside of Google's codebase, so the changes should not be breaking and we can land them in any patch version. The matchers were made accessible in Google's codebase accidentally and we plan to do a cleanup to transition the code to use other helpers once this PR lands. |
|
@AndrewKushnir those fixes are still green without the changes from this PR; I've mailed them to their respective owners and will update here once they are submitted. |
|
@AndrewKushnir 3 CLs submitted: cl/440823458, cl/440891954, cl/440813981 |
|
@toastwaffle great news, thanks for the update! There is a merge conflict with the master branch (due to other changes merged into the master branch). Could you please try to rebase and resolve the conflicts? It'd be great if you could also run a new TGP after that to see if there are any new failures. Thank you. |
…mine Angular now uses Jasmine 2.8 as a minimum version (required by protractor; most of it is using ^3.5.0 which currently resolves to 3.99.0), which makes this shim obsolete - all versions of Jasmine used have Map comparison built in. This shim causes problems for custom equality checkers - when using a Map containing types needing a custom equality check, this fails because the call to `jasmine.matchersUtil.equals` from the shim does not use any of the custom equality matchers.
|
@AndrewKushnir rebased and kicked off a TGP run in cl/441136621 |
|
@AndrewKushnir TGP is green |
|
@toastwaffle thanks for the cleanup! |
AndrewKushnir
removed
the
action: presubmit
|
@toastwaffle This doesn't merge cleanly with |
jessicajaniuk
added
target: minor
|
This PR was merged into the repository by commit c7bf75d. |
|
@jessicajaniuk how often do minor versions land in google3? |
|
@toastwaffle the changes from the master branch are synced into google3 daily. This particular change is in the sync process at this moment. |
|
@toastwaffle It is officially synced now. |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Angular now uses Jasmine 2.8 as a minimum version (required by
protractor; most of it is using ^3.5.0 which currently resolves to
3.99.0), which makes this shim obsolete - all versions of Jasmine used
have Map comparison built in.
This shim causes problems for custom equality checkers - when using a
Map containing types needing a custom equality check, this fails
because the call to
jasmine.matchersUtil.equalsfrom the shim does notuse any of the custom equality matchers.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Jasmine tests comparing equality of Maps containing types requiring custom equality matchers fail
What is the new behavior?
Tests will pass
Does this PR introduce a breaking change?