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

Trigger "changeId" only if content change #4293

Closed
wants to merge 2 commits into from

Conversation

Noodlex
Copy link

@Noodlex Noodlex commented Sep 25, 2024

Trigger "changeId" if the content is modified.

Null or undefined are identical in the process.

@jgonggrijp
Copy link
Collaborator

Could you clarify why you think null and undefined should be considered equivalent? This is different from the semantics of the regular change and change:[attribute] events.

@Noodlex
Copy link
Author

Noodlex commented Sep 26, 2024

It's a proposal.

I have a case where I have a collection in a model.
I receive the complete model with the collection and the new attributes.
These are different and in some cases, I end up with a “prevId” = undefined and a this.id = null.

The behavior seems the same to me in the case of avoiding triggering the “changeId” event, but I'm well aware that undefined is different from null.

@jgonggrijp
Copy link
Collaborator

I have a case where I have a collection in a model. I receive the complete model with the collection and the new attributes. These are different and in some cases, I end up with a “prevId” = undefined and a this.id = null.

To be honest, to me this seems like a situation one would rather avoid in the first place. I am not convinced yet, but maybe you can show me some example code?

@Noodlex
Copy link
Author

Noodlex commented Oct 1, 2024

I don't yet have time to investigate the source of the problem. Currently, this fix on my version allows me to avoid “changeId” trigger problems.

I'll try when I'm less in a rush to find the exact source and maybe add the right fix.

I'll close this request and come back later if needed.

Thanks for your time.

@Noodlex Noodlex closed this Oct 1, 2024
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.

2 participants