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

List Render Bug: when using observables and selectors, there are edge cases where the list stops rendering to the DOM as expected. #1

Closed
ghost opened this issue May 5, 2022 · 7 comments
Labels
bug Something isn't working

Comments

@ghost
Copy link

ghost commented May 5, 2022

Hi, I just found out about this marvelous framework, and I am really loving the look of it and can't wait for its future.

I have spent a little bit of time playing around with observables and selectors to see what they are capable of, and have been extremely happy with them thus far. However, I have run into a small bug with the rendering of lists. It really only happens (as far as I can see) in one or two particular edge cases.

Reproduction repo: https://github.com/JaymanMDev/estrela-issue-reproduction

Reproduction steps:

  1. clone repository and start up development server as usual with vite.
  2. When you see the rendered DOM, a list of numbers will begin to show up on a 1 second interval.
  3. Right clicking one of the list items will remove it from the DOM.
  4. IMPORTANT: Ensure that you click on the first or second list items (before it ever exceeds that amount) in order to reproduce the edge case. No more items should show up if the reproduction worked.
@rosostolato
Copy link
Contributor

Hey @jaymanmdev, I'm really happy to know that you've been working with it and sorry for not have responded earlier.

I've been working on some great improvements for the Estrela framework, some great features that will make it very easy to work with Estrela. I have just pushed a new version today, but still need to fiz some bugs.

I'm going to look into your bug description and see if it's still there, and fix it accordingly.

Thanks so much!

@rosostolato
Copy link
Contributor

About the bug, I'm pretty sure that it's caused by the e.target.remove() function call on click.

When you use an observable as the render content, Estrela creates a fragment which is a collection of nodes within a children list of a Parent node. If the observable children is empty, Estrela creates an empty Text node to serve as a placeholder to keep track of the Parent and position in the children list. If we add an item back to the list, it will replace that Text node with the actual one.

If we remove the node by ourselves, the fragment won't understand that the node was removed and won't be able to patch it later.

I don't know how other languages deal with this, maybe it should have a way to detect these cases, but the correct way would be to remove the node inside the observable data and let it update correctly.

But it's good to have this in mind, I will keep this issue opened to think more about it.

@rosostolato rosostolato added the bug Something isn't working label May 12, 2022
@ghost
Copy link
Author

ghost commented May 12, 2022

Cool! Glad that you were able to track down a probable cause of the issue. It really only happens for a couple edge cases, but otherwise works fine. Just thought I would bring it up with you anyways. Cheers for your time!

@rosostolato
Copy link
Contributor

rosostolato commented May 12, 2022

When you get a chance, please test the latest version. Now it's not necessary to create a state by yourself, just declare a variable using the let keyword and reassign new values to it.

My intention is to make it very simple by removing all the complexity that other frameworks have, like React that requires useState and useEffect.

Checkout the playground package in the repository, I added a Todo example that shows how easy it can be to work with it.

I'm still working on the styled feat that allows css in js, but I needed to remove it because it was not a good idea to have postcss embedded on the code. So to make the examples work again, you need to uncomment the import on index.ts.

@ghost
Copy link
Author

ghost commented May 13, 2022

Hey, I am trying out the new syntax, however, even the minimal counter example on the README doesn't work reactively. And mine is pretty much identical to it.

EDIT: Okay, it works fine using a normal function, however, it breaks if I use an arrow function. Just FYI. :)

@rosostolato
Copy link
Contributor

rosostolato commented May 13, 2022

Okay, it works fine using a normal function, however, it breaks if I use an arrow function. Just FYI. :)

@jaymanmdev how are you writing the arrow function component? Like this?

const App = () => {
  return <div></div>
}

@rosostolato
Copy link
Contributor

Closing the issue. The issue is when you remove a DOM element by yourself and causes it to break Estrela logic, so it shouldn't be covered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant