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.

Sign up for GitHub

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

Jump to bottom

Update MomentJS dependency #9382

Merged
merged 10 commits into from
Apr 22, 2022
Merged

Update MomentJS dependency #9382

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
dacbb1c
Update MomentJS dependency
budnix Apr 7, 2022
5ec0fa6
Updating momentJS fixes an issue with sorting date values
budnix Apr 7, 2022
ea70b74
Fix dateFormat pattern
budnix Apr 8, 2022
14478db
Merge branch 'develop' into feature/snyk-fix-c3454c61088848cba1ba001b…
jansiegel Apr 13, 2022
b93245b
- Correct the date format on the remaining cases in multiColumnSortin…
jansiegel Apr 19, 2022
396c584
Bump moment dependency for the 'handsontable' workspace and the root …
jansiegel Apr 19, 2022
e2eb31c
Merge branch 'develop' into feature/snyk-fix-c3454c61088848cba1ba001b…
jansiegel Apr 20, 2022
a3785fb
Merge branch 'develop' into feature/snyk-fix-c3454c61088848cba1ba001b…
jansiegel Apr 22, 2022
07459a3
Docs: Adding docs changes related to the Moment.js dependency update …
Apr 22, 2022
6d7012d
Merge branch 'develop' into feature/snyk-fix-c3454c61088848cba1ba001b…
jansiegel Apr 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changelogs/9381.json
View file
Open in desktop
@@ -0,0 +1,7 @@
{
"title": "Update MomentJS dependency to 2.29.2.",
"type": "fixed",
"issue": 9381,
"breaking": false,
"framework": "none"
}
2 changes: 1 addition & 1 deletion handsontable/package.json
View file
Open in desktop
Expand Up @@ -79,7 +79,7 @@
"@types/pikaday": "1.7.4",
"core-js": "^3.0.0",
"dompurify": "^2.1.1",
"moment": "2.24.0",
"moment": "2.29.3",
"numbro": "2.1.2",
"pikaday": "1.8.0"
},
Expand Down
22 changes: 11 additions & 11 deletions handsontable/src/plugins/columnSorting/__tests__/columnSorting.spec.js
View file
Open in desktop
Expand Up @@ -1051,7 +1051,7 @@ describe('ColumnSorting', () => {
{},
{
type: 'date',
dateFormat: 'mm/dd/yy'
dateFormat: 'MM/DD/YYYY'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file also uses the old notation dateFormat: 'mm/dd/yy' on lines 294, 1532 (and also in 2 places in multiColumnSorting.spec.js).

Why were other lines not affected?

Copy link
Member

@jansiegel jansiegel Apr 19, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those remaining tests seem to not be sorted by the column containing the dates, so the dateFormat is pretty much redundant, I'll change it, though.

},
{
type: 'numeric'
Expand All @@ -1063,19 +1063,19 @@ describe('ColumnSorting', () => {

getPlugin('columnSorting').sort({ column: 2, sortOrder: 'asc' }); // ASC

expect(getDataAtRow(0)).toEqual(['Mercedes', 'A 160', '01/14/2006', 6999.9999]);
expect(getDataAtRow(1)).toEqual(['Opel', 'Astra', '02/02/2004', 7000]);
expect(getDataAtRow(2)).toEqual(['BMW', '320i Coupe', '07/24/2011', 30500]);
expect(getDataAtRow(3)).toEqual(['Audi', 'A4 Avant', '11/19/2011', 33900]);
expect(getDataAtRow(4)).toEqual(['Citroen', 'C4 Coupe', '12/01/2008', 8330]);
expect(getDataAtRow(0)).toEqual(['Opel', 'Astra', '02/02/2004', 7000]);
Copy link
Member

@warpech warpech Apr 7, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need some root-cause analysis of why is this changed.

Maybe the reviewer (@jansiegel) can explain this? I know that @budnix is busy with other issues.

Copy link
Member Author

@budnix budnix Apr 8, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that before updating the lib, the date values sorting does not work correctly. At least in that test case. For example, the results for sorting rows ascending:

Before (it's sorted without any noticeable pattern):

'01/14/2006'
'02/02/2004'
'07/24/2011'
'11/19/2011'
'12/01/2008'

After:

'02/02/2004'
'01/14/2006'
'12/01/2008'
'07/24/2011'
'11/19/2011'

Copy link
Member

@warpech warpech Apr 8, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sorry, but this is not the root cause. The root cause seems related to the fact that the test, despite its name, provided wrongly formatted data to Moment.js, and tested that Moment.js does not sort in that case. After the change, Moment.js sorts even when it is given wrongly formatted data.

Compare the format given in line 1009 and the data provided in lines 996-1002

handsontable/handsontable/src/plugins/columnSorting/__tests__/columnSorting.spec.js

Lines 993 to 1009 in 5ec0fa6

it('should sort date columns along with empty and null values', () => {
handsontable({
data: [
['Mercedes', 'A 160', '01/14/2006', 6999.9999],
['Citroen', 'C4 Coupe', '12/01/2008', 8330],
['Citroen', 'C4 Coupe null', null, 8330],
['Citroen', 'C4 Coupe empty', '', 8330],
['Audi', 'A4 Avant', '11/19/2011', 33900],
['Opel', 'Astra', '02/02/2004', 7000],
['BMW', '320i Coupe', '07/24/2011', 30500]
],
columns: [
{},
{},
{
type: 'date',
dateFormat: 'mm/dd/yy'

Copy link
Member

@jansiegel jansiegel Apr 8, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's sorted without any noticeable pattern

I think it was sorted kind of as a string, so in this case by the month number

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should use Moment.js in the strict mode? https://momentjs.com/guides/#/parsing/strict-mode/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should use Moment.js in the strict mode? https://momentjs.com/guides/#/parsing/strict-mode/

I would be for it. It's better to only sort the date-type values based on the dateFormat. Without leaving the room for guesses.

Copy link
Member

@warpech warpech Apr 19, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A partial root cause analysis by me:

TL;DR Moment team unknowingly changed the interpretation of incorrect dates. If we used strict, the problem would not appear. Because we didn't use strict, the problem slipped through.

I think we should enforce strict with an explanation in the migration guide.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was advised that there are parts of Handsontable that treat the forgiving mode as a feature and we should not remove it. Discussion: https://handsoncode.slack.com/archives/C4PV4S33Q/p1650384660407479?thread_ts=1649328227.856499&cid=C4PV4S33Q

expect(getDataAtRow(1)).toEqual(['Mercedes', 'A 160', '01/14/2006', 6999.9999]);
expect(getDataAtRow(2)).toEqual(['Citroen', 'C4 Coupe', '12/01/2008', 8330]);
expect(getDataAtRow(3)).toEqual(['BMW', '320i Coupe', '07/24/2011', 30500]);
expect(getDataAtRow(4)).toEqual(['Audi', 'A4 Avant', '11/19/2011', 33900]);

getPlugin('columnSorting').sort({ column: 2, sortOrder: 'desc' }); // DESC

expect(getDataAtRow(0)).toEqual(['Citroen', 'C4 Coupe', '12/01/2008', 8330]);
expect(getDataAtRow(1)).toEqual(['Audi', 'A4 Avant', '11/19/2011', 33900]);
expect(getDataAtRow(2)).toEqual(['BMW', '320i Coupe', '07/24/2011', 30500]);
expect(getDataAtRow(3)).toEqual(['Opel', 'Astra', '02/02/2004', 7000]);
expect(getDataAtRow(4)).toEqual(['Mercedes', 'A 160', '01/14/2006', 6999.9999]);
expect(getDataAtRow(0)).toEqual(['Audi', 'A4 Avant', '11/19/2011', 33900]);
expect(getDataAtRow(1)).toEqual(['BMW', '320i Coupe', '07/24/2011', 30500]);
expect(getDataAtRow(2)).toEqual(['Citroen', 'C4 Coupe', '12/01/2008', 8330]);
expect(getDataAtRow(3)).toEqual(['Mercedes', 'A 160', '01/14/2006', 6999.9999]);
expect(getDataAtRow(4)).toEqual(['Opel', 'Astra', '02/02/2004', 7000]);
});
});

Expand Down
26 changes: 13 additions & 13 deletions handsontable/src/plugins/multiColumnSorting/__tests__/multiColumnSorting.spec.js
View file
Open in desktop
Expand Up @@ -276,7 +276,7 @@ describe('MultiColumnSorting', () => {
{},
{
type: 'date',
dateFormat: 'mm/dd/yy'
dateFormat: 'MM/DD/YY'
},
{
type: 'numeric'
Expand Down Expand Up @@ -973,7 +973,7 @@ describe('MultiColumnSorting', () => {
{},
{
type: 'date',
dateFormat: 'mm/dd/yy'
dateFormat: 'MM/DD/YYYY'
},
{
type: 'numeric'
Expand All @@ -985,19 +985,19 @@ describe('MultiColumnSorting', () => {

getPlugin('multiColumnSorting').sort({ column: 2, sortOrder: 'asc' }); // ASC

expect(getDataAtRow(0)).toEqual(['Mercedes', 'A 160', '01/14/2006', 6999.9999]);
expect(getDataAtRow(1)).toEqual(['Opel', 'Astra', '02/02/2004', 7000]);
expect(getDataAtRow(2)).toEqual(['BMW', '320i Coupe', '07/24/2011', 30500]);
expect(getDataAtRow(3)).toEqual(['Audi', 'A4 Avant', '11/19/2011', 33900]);
expect(getDataAtRow(4)).toEqual(['Citroen', 'C4 Coupe', '12/01/2008', 8330]);
expect(getDataAtRow(0)).toEqual(['Opel', 'Astra', '02/02/2004', 7000]);
expect(getDataAtRow(1)).toEqual(['Mercedes', 'A 160', '01/14/2006', 6999.9999]);
expect(getDataAtRow(2)).toEqual(['Citroen', 'C4 Coupe', '12/01/2008', 8330]);
expect(getDataAtRow(3)).toEqual(['BMW', '320i Coupe', '07/24/2011', 30500]);
expect(getDataAtRow(4)).toEqual(['Audi', 'A4 Avant', '11/19/2011', 33900]);

getPlugin('multiColumnSorting').sort({ column: 2, sortOrder: 'desc' }); // DESC

expect(getDataAtRow(0)).toEqual(['Citroen', 'C4 Coupe', '12/01/2008', 8330]);
expect(getDataAtRow(1)).toEqual(['Audi', 'A4 Avant', '11/19/2011', 33900]);
expect(getDataAtRow(2)).toEqual(['BMW', '320i Coupe', '07/24/2011', 30500]);
expect(getDataAtRow(3)).toEqual(['Opel', 'Astra', '02/02/2004', 7000]);
expect(getDataAtRow(4)).toEqual(['Mercedes', 'A 160', '01/14/2006', 6999.9999]);
expect(getDataAtRow(0)).toEqual(['Audi', 'A4 Avant', '11/19/2011', 33900]);
expect(getDataAtRow(1)).toEqual(['BMW', '320i Coupe', '07/24/2011', 30500]);
expect(getDataAtRow(2)).toEqual(['Citroen', 'C4 Coupe', '12/01/2008', 8330]);
expect(getDataAtRow(3)).toEqual(['Mercedes', 'A 160', '01/14/2006', 6999.9999]);
expect(getDataAtRow(4)).toEqual(['Opel', 'Astra', '02/02/2004', 7000]);
});
});

Expand Down Expand Up @@ -1452,7 +1452,7 @@ describe('MultiColumnSorting', () => {
{},
{
type: 'date',
dateFormat: 'mm/dd/yy'
dateFormat: 'MM/DD/YY'
},
{
type: 'numeric'
Expand Down