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

Data in Task and TasksDate are accidentally mutable - dates can be edited unintentionally #2866

Open
2 of 7 tasks
claremacrae opened this issue May 30, 2024 · 4 comments
Open
2 of 7 tasks
Labels
priority: high A high priority/important request scope: task dates and times Requests for enhancements to types and formats of dates and times associated with tasks type: bug Something isn't working

Comments

@claremacrae
Copy link
Collaborator

Please check that this issue hasn't been reported before.

  • I searched previous Bug Reports didn't find any similar reports.

Expected Behavior

That a user should be able to do task.due.moment?.startOf('week') or similar in group by function, and not modify the stored date in the task.

Cause: https://momentjscom.readthedocs.io/en/latest/moment/03-manipulating/03-start-of/

Mutates the original moment by setting it to the start of a unit of time.

There may be other methods of Moment that mutate the stored data.

I suspect that this may be the underlying cause of:

Based on the tests below, I expect that doing so would modify the stored data in Task, even though Task is designed to be immutable.

Also, that these tests should pass:

describe('immutability', () => {
    it.failing('should not be possible to edit a date Moment after Task creation', () => {
        // TODO Make Task's use of Moment immutable - always return a clone of the stored Moment.
        //      https://momentjscom.readthedocs.io/en/latest/moment/01-parsing/12-moment-clone/

        const inputDate = '2024-02-28 12:34';
        const task = new Task({ ...new TaskBuilder().build(), dueDate: moment(inputDate) });

        const parsedDate = '2024-02-28T12:34:00.000Z';
        expect(task.dueDate).toEqualMoment(moment(parsedDate));

        task.dueDate?.startOf('day');
        // TODO This fails, giving '2024-02-28T00:00:00.000Z', as the startOf call edits the stored date.
        //      See https://www.geeksforgeeks.org/moment-js-moment-startof-method/.
        expect(task.dueDate).toEqualMoment(moment(parsedDate));
    });

    it.failing('should not be possible to edit a date TasksDate after Task creation', () => {
        // TODO Make TasksDate objects immutable - always return a clone of the stored Moment.
        //      https://momentjscom.readthedocs.io/en/latest/moment/01-parsing/12-moment-clone/

        const inputDate = '2024-02-28 12:34';
        const task = new Task({ ...new TaskBuilder().build(), dueDate: moment(inputDate) });

        const due: TasksDate = task.due;
        const parsedDate = '2024-02-28T12:34:00.000Z';
        expect(due.moment).toEqualMoment(moment(parsedDate));

        due.moment?.startOf('day');
        // TODO This fails, giving '2024-02-28T00:00:00.000Z', as the startOf call edits the stored date.
        //      See https://www.geeksforgeeks.org/moment-js-moment-startof-method/.
        expect(due.moment).toEqualMoment(moment(parsedDate));
    });
});

Current behaviour

The above tests fail, and so are marked as .failing.

Steps to reproduce

See the tests above, which I will commit.

Which Operating Systems are you using?

  • Android
  • iPhone/iPad
  • Linux
  • macOS
  • Windows

Obsidian Version

1.6.1

Tasks Plugin Version

7.3.0

Checks

  • I have tried it with all other plugins disabled and the error still occurs

Possible solution

The code should always clone moment objects before returning them.

See https://momentjscom.readthedocs.io/en/latest/moment/01-parsing/12-moment-clone/

@claremacrae claremacrae added type: bug Something isn't working scope: task dates and times Requests for enhancements to types and formats of dates and times associated with tasks labels May 30, 2024
@claremacrae
Copy link
Collaborator Author

I've committed the tests in description above: 3db9d43.

@claremacrae claremacrae changed the title Data in Task and TasksDate are accidentally not immutable - dates can be edited unintenionally Data in Task and TasksDate are accidentally mutable - dates can be edited unintenionally Jun 6, 2024
@claremacrae claremacrae added the priority: high A high priority/important request label Jun 22, 2024
@claremacrae
Copy link
Collaborator Author

I discovered today that the 'moment.js Project Status' page specifically mentions this:

As an example, consider that Moment objects are mutable. This is a common source of complaints about Moment. We address it in our usage guidance but it still comes as a surprise to most new users. Changing Moment to be immutable would be a breaking change for every one of the projects that use it. Creating a "Moment v3" that was immutable would be a tremendous undertaking and would make Moment a different library entirely. Since this has already been accomplished in other libraries, we feel that it is more important to retain the mutable API.

@claremacrae
Copy link
Collaborator Author

@claremacrae claremacrae changed the title Data in Task and TasksDate are accidentally mutable - dates can be edited unintenionally Data in Task and TasksDate are accidentally mutable - dates can be edited unintentionally Jul 30, 2024
@claremacrae
Copy link
Collaborator Author

Whilst looking at this, consider the recommendations in:
Date and Time Odds, Ends and Oddities

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: high A high priority/important request scope: task dates and times Requests for enhancements to types and formats of dates and times associated with tasks type: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant