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

Reset the cache of the last modified key when the form data is reset or updated externally #1602

Merged
merged 2 commits into from
Apr 4, 2023

Conversation

atf-cguerin
Copy link
Contributor

This is the same patch proposed as a fix for #1599 converted into a pull request after a little more internal testing, in case it can simplify the merging process if it's approved.

@Pagebakers
Copy link

When can we expect this to be merged?

This has been an issues for a couple of years already where change events of inputs (select/radio) aren't triggered after updating the form sourceDoc.

@Pagebakers
Copy link

We could also bind the keyVal to the docId if it's available

keyVal = key + '___' + keyVal;

if (this.docId) {
keyVal = this.docId + '___' + keyVal
}
if (formId in lastKeyVals && keyVal === lastKeyVals[formId]) {
      return;
    }

@jankapunkt jankapunkt closed this Feb 18, 2021
@velatinicolas
Copy link

Hello @jankapunkt, I and my work team are currently facing the same issue as described above, and we eventually stumbled upon this pull request which seems to perfectly fix the bug. Do you have any explanation on why it has not been merged? Would it be conceivable to merge it now, even if it has been closed 2 years ago?

@jankapunkt
Copy link
Member

@velatinicolas I think it has been closed due to being stale for a long time and at that time I was not able to review all PRs. Feel free to reopen and assign me for review, today I will have some time for OS dev 😀

@jankapunkt jankapunkt reopened this Mar 31, 2023
@jankapunkt
Copy link
Member

@velatinicolas okay I now know why I closed, mainly because it introduces a global-scoped variable which is nowadays considered an anti-pattern.

I could extend the PR by rewriting this to be within the AutoForm namespace:

AutoForm._lastKeyVals = {}

wdyt?

@velatinicolas
Copy link

First of all, thanks for your quick answers and propositions! 🚀

I agree, the global variable bothered me a bit as well. Including it in the AutoForm namespace looks great.

But actually, after some tests I made today, for our needs we'd only need to do the fix in the autoform-events.js file, because our problem occurs on a form reset. I've not been able to reproduce the bug initially described in the issue #1599.

As you are the maintainer of this project, I'll let you decide on this. Either we apply the fix in both autoform-events.js and autoForm.js file, therefore requiring the rework with the namespace ; or we keep the lastKeyVals variable declared with const and only fix autoform-events.js.

@jankapunkt jankapunkt merged commit 1d91a93 into Meteor-Community-Packages:devel Apr 4, 2023
@velatinicolas
Copy link

Thanks for the merge @jankapunkt 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants