reimplement noncommutative sort #2
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
|
👍 looks good to me |
amireh
added a commit
to instructure/i18nliner-js
that referenced
this pull request
May 24, 2022
refs FOO-1116 flag = none this pulls in a version of the package that can work under node > 10, see instructure/i18nliner-handlebars#2 :: test plan you need to try this in both node 10 and node 12, run webpack when you switch versions create a module in a course that has some students and add some stuff to it, then view its progress and verify the student names show up fine Change-Id: I5f5b110b64cc6ffab3dc190785c641c0b3bc5b4a Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/253070 Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com> Reviewed-by: Simon Williams <simon@instructure.com> QA-Review: Simon Williams <simon@instructure.com> Product-Review: Ahmad Amireh <ahmad@instructure.com>
amireh
added a commit
to instructure/i18nliner-js
that referenced
this pull request
May 25, 2022
refs FOO-1116 flag = none this pulls in a version of the package that can work under node > 10, see instructure/i18nliner-handlebars#2 :: test plan you need to try this in both node 10 and node 12, run webpack when you switch versions create a module in a course that has some students and add some stuff to it, then view its progress and verify the student names show up fine Change-Id: I5f5b110b64cc6ffab3dc190785c641c0b3bc5b4a Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/253070 Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com> Reviewed-by: Simon Williams <simon@instructure.com> QA-Review: Simon Williams <simon@instructure.com> Product-Review: Ahmad Amireh <ahmad@instructure.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
changed1
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