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

reimplement noncommutative sort #1

Closed
wants to merge 1 commit into from
Closed

Conversation

amireh
Copy link

@amireh amireh commented Nov 18, 2020

the workaround for handlebars-lang/handlebars.js/issues/748 employed a sorting
routine that was not commutative and with the v8 upgrade in node11, the
order of the arguments changed (and is considered an implementation
detail that we're not meant to rely on) and so the routine would yield
surprising results

this patch reimplements that sorting routine to always yield the same
result regardless of the order of the operands, so should work on node <
11 and > 11

test plan: there is already a case for this, so I just added node12 to
the travis language matrix

@CLAassistant
Copy link

CLAassistant commented Nov 18, 2020

CLA assistant check
All committers have signed the CLA.

the workaround for handlebars-lang/handlebars.js/issues/748 employed a sorting
routine that was not commutative; its output relied on the order of its
arguments, and with the V8 engine upgrade in node11, that order has
changed[1]

the order of the arguments to Array#sory is considered an implementation
detail that we can't rely on

this patch reimplements that sorting routine to always return the same
result regardless of the order of its operands, and so it should work on
node <= 10 and >= 11

test plan: there is already a case for this, so I just added node12 to
           the travis language matrix

[1]: nodejs/node#24294
@amireh amireh closed this Nov 18, 2020
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