Skip to content

datepicker: memory leak#1852

Merged
fnagel merged 1 commit into
jquery:masterfrom
jigar140291:BUG/memory-leak-in-datepicker
Mar 23, 2020
Merged

datepicker: memory leak#1852
fnagel merged 1 commit into
jquery:masterfrom
jigar140291:BUG/memory-leak-in-datepicker

Conversation

@jigar140291
Copy link
Copy Markdown
Contributor

@jigar140291 jigar140291 commented Jan 31, 2018 •
edited
Loading

Ticket: 15250

@jsf-clabot
Copy link
Copy Markdown

jsf-clabot commented Jan 31, 2018 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@jigar140291 jigar140291 force-pushed the BUG/memory-leak-in-datepicker branch from 8d95a3d to 543fa24 Compare January 31, 2018 22:19
@fnagel fnagel self-assigned this Dec 10, 2019
@fnagel
Copy link
Copy Markdown
Member

fnagel commented Dec 10, 2019

Hey @jigar140291 thanks for your contribution and sorry for the delay.

Sounds reasonable to me and I'm able to debug this, but not within memory debugger functionality. Can you help me out here?

@arschmitz What do you think about this?

mgol
mgol reviewed Dec 30, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

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

This makes sense to me but it'd perhaps be good to have a unit test verifying the field is cleared.

@fnagel
Copy link
Copy Markdown
Member

fnagel commented Jan 2, 2020

@jigar140291 Would you mind adding a test for this?

@jigar140291
Copy link
Copy Markdown
Contributor Author

jigar140291 commented Jan 2, 2020

@fnagel Sorry for the delay. I will look into the test cases.

@jigar140291
Copy link
Copy Markdown
Contributor Author

jigar140291 commented Jan 6, 2020

@fnagel I have added test cases. could you please review it.

mgol
mgol reviewed Jan 10, 2020
View reviewed changes
Comment thread tests/unit/datepicker/methods.js Outdated
@jigar140291
Copy link
Copy Markdown
Contributor Author

jigar140291 commented Jan 10, 2020

@mgol unit test case updated.

mgol
mgol previously approved these changes Jan 10, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

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

Looks good to me! @fnagel @arschmitz what do you think?

@fnagel fnagel self-requested a review January 14, 2020 08:58
fnagel
fnagel requested changes Jan 14, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@fnagel fnagel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me! Only one thing left before merging:

  • Squash commit into one
  • Improve commit message (e.g. "Datepicker: Fix current instance memory leak and add tests")

I can do this for you too if needed.

jigar140291
jigar140291 commented Jan 14, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@jigar140291 jigar140291 left a comment

Choose a reason for hiding this comment

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

Working on it. Thanks for the feedback.

@jigar140291
Copy link
Copy Markdown
Contributor Author

jigar140291 commented Jan 14, 2020

@fnagel it's done. could you please review and merge.

@fnagel
Copy link
Copy Markdown
Member

fnagel commented Jan 14, 2020

@jigar140291 Sorry but now there are even more commits. Wanna try again or should I do this for you?

@jigar140291
Copy link
Copy Markdown
Contributor Author

jigar140291 commented Jan 14, 2020

@fnagel seems like I squash it incorrectly. Could you do that for me. Thanks!!

@jigar140291 jigar140291 force-pushed the BUG/memory-leak-in-datepicker branch from ef4f20e to a35c0b1 Compare January 19, 2020 15:41
@jigar140291
Copy link
Copy Markdown
Contributor Author

jigar140291 commented Jan 19, 2020

@fnagel I have squashed commits into one single commit. please check and merge.

@fnagel fnagel requested review from arschmitz and fnagel January 20, 2020 15:44
@jigar140291
Copy link
Copy Markdown
Contributor Author

jigar140291 commented Jan 27, 2020

@arschmitz Could you please review and merge it.

@fnagel fnagel merged commit 817ce38 into jquery:master Mar 23, 2020
@mgol mgol added this to the 1.13 milestone Jun 10, 2020
@mgol mgol mentioned this pull request Aug 1, 2024
Merged
@mgol mgol mentioned this pull request Aug 14, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fnagel fnagel fnagel approved these changes

@arschmitz arschmitz arschmitz approved these changes

@mgol mgol mgol left review comments

Assignees

@fnagel fnagel

Labels

None yet

Milestone

1.13.0

Development

Successfully merging this pull request may close these issues.

5 participants

@jigar140291 @jsf-clabot @fnagel @arschmitz @mgol