Update MomentJS dependency #9382

budnix · 2022-04-07T11:01:37Z

Context

The PR upgrades the moment library to 2.29.2. The PR applies all changes from the origin PR added by the bot (#9376), adds necessary changelog entry, and updates the package-lock.json file.

Migration guide

If I remember correctly, some of the users that use the MomentJS library within their apps struggle with an issue where the library was double transpiled in the bundle file. One version for the developer's app and one for Handsontable dependency. The workaround for that was using the same version of the library as Handsontable.

The PR changes the version, so it can be advisable to mention this in the migration guide.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Related issue(s):

Affected project(s):

handsontable

Checklist:

I have reviewed the guidelines about Contributing to Handsontable and I confirm that my code follows the code style of this project.
I have signed the Contributor License Agreement

Origin: #9376

The commit fixes the tests.

warpech · 2022-04-07T14:25:59Z

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

-      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]);


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.

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'

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'

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

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

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.

A partial root cause analysis by me:

in the forgiving mode, if Moment cannot parse a date, it uses Date constructor

in Moment 2.24.0, the date moment('01/14/2006', 'mm/dd/yy') was not parsed at all. The Date constructor fallback returned the current datetime (https://jsfiddle.net/warpech/d9qako0g/4/)

in Moment 2.25.0 (actually in this commit moment/moment@0c7d8da, a squashed version of [feature] Add era support for en,ja (closes #4591) moment/moment#4599), the character yy became significant and that date was parsed without resolving to the Date constructor. However, parsing is not good either. Moment parses this date to 2006-01-01. (https://jsfiddle.net/warpech/d9qako0g/4/)

with strict flag turned off, all tested Moment versions from 2.24.0 to 2.29.2 consistently say it is an incorrect date.

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.~~

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

warpech · 2022-04-08T13:16:43Z

We discussed on the today's daily (https://kb.handsontable.com/pages/viewpage.action?pageId=56461739) that @jansiegel will take over this PR.

According to the notes from the daily, we need:

much more tests to see what's changed
specification what should happen when data does not match dateFormat (awaiting @krzysztofspilka)

…fb76870c

warpech · 2022-04-19T11:06:03Z

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

@@ -1051,7 +1051,7 @@ describe('ColumnSorting', () => {
          {},
          {
            type: 'date',
-            dateFormat: 'mm/dd/yy'
+            dateFormat: 'MM/DD/YYYY'


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?

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.

…g.spec.js.

…directory.

…fb76870c

kirszenbaum

@jansiegel – I'm adding my changes in #9409

Just checking: should we update anything in the correctFormat API ref? https://handsontable.com/docs/api/options/#correctformat

jansiegel · 2022-04-22T06:40:45Z

@kirszenbaum

Just checking: should we update anyting in the correctFormat API ref? https://handsontable.com/docs/api/options/#correctformat

I don't think so - our API didn't change, what has changed is some logic on Moment's side, so until we have a more detailed specification on what exactly we should allow being automatically corrected, I'd leave it as it is.

…fb76870c

…9409)

github-actions · 2022-04-22T08:31:52Z

Launch the local version of documentation by running:

npm run docs:review 90dfc4f1d2b207457890fd44d2ff32ab27031c72

…fb76870c

* Update MomentJS dependency Origin: #9376 * Updating momentJS fixes an issue with sorting date values The commit fixes the tests. * Fix dateFormat pattern * - Correct the date format on the remaining cases in multiColumnSorting.spec.js. * Bump moment dependency for the 'handsontable' workspace and the root directory. * Docs: Adding docs changes related to the Moment.js dependency update (#9409) Co-authored-by: Jan Siegel <jansiegel@users.noreply.github.com> Co-authored-by: Jan Siegel <js.ziggle@gmail.com> Co-authored-by: Kuba Jakub <jakub.wisniewski@handsontable.com>

Update MomentJS dependency

dacbb1c

Origin: #9376

budnix self-assigned this Apr 7, 2022

budnix marked this pull request as ready for review April 7, 2022 11:09

Updating momentJS fixes an issue with sorting date values

5ec0fa6

The commit fixes the tests.

budnix requested review from jansiegel and kirszenbaum April 7, 2022 11:48

warpech reviewed Apr 7, 2022

View reviewed changes

Fix dateFormat pattern

ea70b74

warpech mentioned this pull request Apr 8, 2022

Moment library update, adapt to the changes in Moment's parsing of dates #9381

Closed

jansiegel mentioned this pull request Apr 8, 2022

Our date/time cell type tests don't cover many cases. #9387

Closed

jansiegel removed their request for review April 12, 2022 14:19

jansiegel assigned jansiegel and unassigned budnix Apr 12, 2022

Merge branch 'develop' into feature/snyk-fix-c3454c61088848cba1ba001b…

14478db

…fb76870c

warpech mentioned this pull request Apr 13, 2022

[Snyk] Security upgrade moment from 2.24.0 to 2.29.2 #9376

Closed

warpech reviewed Apr 19, 2022

View reviewed changes

jansiegel added 2 commits April 19, 2022 13:12

- Correct the date format on the remaining cases in multiColumnSortin…

b93245b

…g.spec.js.

Bump moment dependency for the 'handsontable' workspace and the root …

396c584

…directory.

jansiegel requested a review from warpech April 20, 2022 07:47

Merge branch 'develop' into feature/snyk-fix-c3454c61088848cba1ba001b…

e2eb31c

…fb76870c

kirszenbaum requested changes Apr 21, 2022

View reviewed changes

jansiegel and others added 2 commits April 22, 2022 08:40

Merge branch 'develop' into feature/snyk-fix-c3454c61088848cba1ba001b…

a3785fb

…fb76870c

Docs: Adding docs changes related to the Moment.js dependency update (#…

07459a3

…9409)

kirszenbaum self-requested a review April 22, 2022 08:25

kirszenbaum approved these changes Apr 22, 2022

View reviewed changes

Merge branch 'develop' into feature/snyk-fix-c3454c61088848cba1ba001b…

6d7012d

…fb76870c

jansiegel merged commit 90dfc4f into develop Apr 22, 2022

jansiegel deleted the feature/snyk-fix-c3454c61088848cba1ba001bfb76870c branch April 22, 2022 09:28

wszymanski mentioned this pull request Jul 19, 2022

[12.0.0+] Incorrect date format conversion on input #9681

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update MomentJS dependency #9382

Update MomentJS dependency #9382

budnix commented Apr 7, 2022 •

edited

warpech Apr 7, 2022 •

edited

budnix Apr 8, 2022 •

edited

warpech Apr 8, 2022 •

edited

jansiegel Apr 8, 2022 •

edited

warpech Apr 8, 2022

budnix Apr 8, 2022

warpech Apr 19, 2022 •

edited

warpech Apr 19, 2022

warpech commented Apr 8, 2022

warpech Apr 19, 2022

jansiegel Apr 19, 2022 •

edited

kirszenbaum left a comment •

edited

jansiegel commented Apr 22, 2022

github-actions bot commented Apr 22, 2022 •

edited

	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'

Update MomentJS dependency #9382

Update MomentJS dependency #9382

Conversation

budnix commented Apr 7, 2022 • edited

Context

Migration guide

Types of changes

Related issue(s):

Affected project(s):

Checklist:

warpech Apr 7, 2022 • edited

Choose a reason for hiding this comment

budnix Apr 8, 2022 • edited

Choose a reason for hiding this comment

warpech Apr 8, 2022 • edited

Choose a reason for hiding this comment

jansiegel Apr 8, 2022 • edited

Choose a reason for hiding this comment

warpech Apr 8, 2022

Choose a reason for hiding this comment

budnix Apr 8, 2022

Choose a reason for hiding this comment

warpech Apr 19, 2022 • edited

Choose a reason for hiding this comment

warpech Apr 19, 2022

Choose a reason for hiding this comment

warpech commented Apr 8, 2022

warpech Apr 19, 2022

Choose a reason for hiding this comment

jansiegel Apr 19, 2022 • edited

Choose a reason for hiding this comment

kirszenbaum left a comment • edited

Choose a reason for hiding this comment

jansiegel commented Apr 22, 2022

github-actions bot commented Apr 22, 2022 • edited

budnix commented Apr 7, 2022 •

edited

warpech Apr 7, 2022 •

edited

budnix Apr 8, 2022 •

edited

warpech Apr 8, 2022 •

edited

jansiegel Apr 8, 2022 •

edited

warpech Apr 19, 2022 •

edited

jansiegel Apr 19, 2022 •

edited

kirszenbaum left a comment •

edited

github-actions bot commented Apr 22, 2022 •

edited