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

Filtering out non-significiant changes Palindrom behavior #58

Open
tomalec opened this issue Sep 5, 2019 · 1 comment
Open

Filtering out non-significiant changes Palindrom behavior #58

tomalec opened this issue Sep 5, 2019 · 1 comment

Comments

@tomalec
Copy link
Member

tomalec commented Sep 5, 2019

@warpech, @eriksunsol #45 changes Palindrom behavior in respect of numbers > Number.MAX_SAFE_INTEGER.
This change checks that Number.MAX_SAFE_INTEGER == Number.MAX_SAFE_INTEGER +1 // true and do not emit a patch, therefore such error is not cough by Palindrom's validation.
And the tests that are failing are:

HTTP :
WebSocket:
Outgoing patch: out of range numbers should call onOutgoingPatchValidationError event with a RangeError

WDYT of

  1. adding Number range validation to JSONPatcherProxy?
  2. Removing Range validation for outgoing patches from Palindrom?

as I cannot think of any other solution.

You can check the behavior at https://github.com/Palindrom/Palindrom/compare/update-deps?expand=1
https://travis-ci.org/Palindrom/Palindrom/builds/581106763

tomalec added a commit to Palindrom/Palindrom that referenced this issue Sep 5, 2019
@tomalec tomalec changed the title Filtering out non-significiant changes breaks Palindrom Filtering out non-significiant changes Palindrom behavior Sep 5, 2019
@warpech
Copy link
Contributor

warpech commented Sep 5, 2019

I vote for option 2 with clarification in JSONPatcherProxy and Palindrom docs.

Reasoning: Anyone who uses numbers above MAX_SAFE_INTEGER in JavaScript should expect the unexpected. Since JS can't recognize a difference between two numbers, I don't think that JSONPatcherProxy or Palindrom should do that either.

Option 1 would be a performance degradation.

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

No branches or pull requests

2 participants