Skip to content

fix(react): debounce onload events - #137

Merged
stephancill merged 1 commit into
mainfrom
fix/onload-http-infinite-reqs
Jan 5, 2024
Merged

fix(react): debounce onload events#137
stephancill merged 1 commit into
mainfrom
fix/onload-http-infinite-reqs

Conversation

@stephancill

Copy link
Copy Markdown
Contributor

Change Summary

Fixes #135 by introducing a state variable which debounces the call to onload events

Merge Checklist

  • PR has a changeset
  • PR includes documentation if necessary
  • PR updates the rich-embed examples if necessary
  • includes a parallel PR for Mod-starter and the gateway if necessary

@changeset-bot

changeset-bot Bot commented Dec 23, 2023

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 4c8e600

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@mod-protocol/react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel

vercel Bot commented Dec 23, 2023

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
api ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 5, 2024 0:02am
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 5, 2024 0:02am
example-nextjs-shadcn ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 5, 2024 0:02am

Comment thread packages/react/src/index.tsx Outdated
events.onLoad();
}, [events]);
if (!loaded) {
events.onLoad();

@davidfurlong davidfurlong Dec 25, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

isn't the real issue here that this useEffect is being triggered again when it shouldn't? the only way for that to happen is if the events object is different - and it seems its different unnecessarily? perhaps we're not memoizing it before passing it to this component when we should?

@stephancill stephancill Jan 2, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I wonder if it's coming from reconstructing the anonymous function here when mapCurrentTree is called

events: {
onLoad: () => {
if (el.onload) {
this.stepIntoOrTriggerAction(el.onload);
}
},
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

not sure - I think perhaps our core renderer doesn't follow react's component model with uniquely deduplicated components (keys) vs just remounting all the components. Could be worth passing a key or trying to debug whether thats what the cause is.

@stephancill stephancill Jan 4, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

did a bit of reading and useEffect compares reference types in the dependency array based on their location in memory. so what i think is happening is once the tree rerenders when the context changes, it creates a new anonymous function which will have a new location in memory and hence retriggers the useEffect hook

when i change the latch to a simple state variable that takes on the initial value of the events prop, it doesn't do the infinite looping anymore:

const [events] = React.useState(element.events);

React.useEffect(() => {
  events.onLoad();
}, [events]);

const { events, type, elements, ...rest } = element;
const { type, elements, ...rest } = element;

const [events] = React.useState(element.events);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

a comment would be helpful. otherwise lgtm if events are immutable over the lifecycle of a component, which I think they are

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.

http request is executed infinitely when specified in onload on a root mod element

2 participants