Feedback after creating a simple app

[lukejagodzinski]

Hey, I've created a simple app for renaming image files using Crank.js that you can checkout here https://files.jagi.io/ and the source code is here https://github.com/jagi/files

And here is a little bit of feedback that I have after spending some time with it. It's still very, very simple app, so I will probably have more to say when I start adding more features like dialog windows, forms etc. But still I already have some feedback.

GOOD:

1. Overall an experience of working with Crank.js was great. Integrating it with the tools that I like and use with React was easy.
2. Not having to use camelCase property names was a nice addon especially when not having to "fix" property names. It will be handy when working with SVGs.
3. Ability to use async function directly in the components was a game changer. It just made developing app so much nicer. However, I stumbled upon a problem when I needed to await for all promises to resolve and then do some operations. Of course Promise.all is the way to go but I just had to switch my thinking from sync to async components. So it's just a problem of someone coming from React environment. https://github.com/jagi/files/blob/main/src/components/table.tsx#L108-L120
4. Using this.addEventListener to handle events is great. Again, just because I'm coming from React, I had to change my habits. I remember, that once I wanted to handle two events from two elements in one component and I didn't like switches/ifs in the event handler but then I've just split it into two smaller components. So I think it's good to think about this.addEventListener as the handler for the top level element in the component. So far, I haven't had problem with this approach.

BAD:

1. App state management with generators. I don't know why but I just dislike it. It's maybe because of having to "define" variables twice. Once just define/declare them at the top https://github.com/jagi/files/blob/main/src/components/state.tsx#L43-L47 and later user this.provide to expose them to children https://github.com/jagi/files/blob/main/src/components/state.tsx#L57-L61. Also in some cases, having to update both of them when plain value changes https://github.com/jagi/files/blob/main/src/components/state.tsx#L64-L65 although not having to do it when working with references https://github.com/jagi/files/blob/main/src/components/state.tsx#L75. I will probably end up rewriting this part to use something like Zustand and creating Crank wrapper or just creating my own app state management library for Crank. I don't have problems with local component state but app state management is far from perfect.
2. Having to write long dispatchEvent calls:

this.dispatchEvent(
  new CustomEvent("app.add-directory", {
    bubbles: true,
    detail: { directory },
  })
);

So I ended up creating classes for each event type like this:

export class AddDirectoryEvent extends CustomEvent<{directory: FileSystemDirectoryHandle}> {
  constructor(directory: FileSystemDirectoryHandle) {
    super("app.add-directory", { bubbles: true, detail: { directory } });
  }
}

declare global {
  module Crank {
    interface EventMap {
      "app.add-directory": AddDirectoryEvent;
    }
  }
}

and then using it is much cleaner:

this.dispatchEvent(new OpenDirectoryEvent(fileOrDirectory));

So actually that one I kinda solved by myself and I'm happy with it. But just saying that I (and probably most people) like the idea of writing less code :) especially when you used some other state management library.

Sometimes I also had issue with components not refreshing but maybe that was just my bug in the code. I have to investigate it more and confirm. Sometimes it would be good to batch this.refresh calls.

I know that this feedback is nothing mind blowing and you probably are aware of all of this, but just wanted to share my experience.

[brainkim]

The app is great! I’ve been following the file system access standard closely so it’s exciting to see an app which uses it!

My thoughts:

Defining CustomEvent subclasses is a great idea! I always forget to bubble custom events, and having a single file where all the custom events of your application are defined feels clean.

---

I’m not 100% on why the Table component needs to have async render props. Personally, I would probably have a single async function which turns an array of FileSystemHandle objects into the data structure you want for the table. This approach would be framework agnostic.

I typically try to avoid render/callback props whenever I can. Maybe you have good reasons for implementing things your way.

---

As far as the State component goes, I think you could probably make some refactorings to make your life a little easier. First, you should probably split the light/dark mode logic from the the file/directory logic. Additionally, if you’re relying on experimental browser features, you could probably also get away with implementing light/dark mode with custom css properties, though I’m not sure how that works with JSS.

Second, the other file/directory provisions all seem related, and when consumed, you seem to consume multiple of these provisions at once. Therefore, I recommend putting these 4 provisions into a single object, or maybe a helper class.

Finally, provide/consume is just one way to have cross-component logic; you could also maybe move the data into a global which is shared, via one the strategies I list here (https://crank.js.org/guides/reusable-logic#strategies-for-reusing-logic). The question you have to ask is, does it make sense for there to be multiple State components in the same tree? If not, and you’re merely providing state via a single component at the root, what benefit is there over using some sort of global or singleton? Again using an approach like that would be framework agnostic, and potentially cleaner.

---

Sometimes I also had issue with components not refreshing but maybe that was just my bug in the code. I have to investigate it more and confirm. Sometimes it would be good to batch this.refresh calls.

Yes, this is going to be a problem for Crank (see the discussion in #37). I can’t guarantee that components failing to update is because of user error and not a bug in Crank itself, and I must admit that I also often fail to remember to refresh components, but I still believe explicit, opt-in refreshing is the best approach. I have a blog post in the works about why I think this is the case, but for now know that it is an intentional decision of the framework to under-render, as opposed to over-render, because it makes the executions of your components clearer, it helps avoid pathological performance problems when applications reach a certain size, and because it allows for advanced rendering patterns (like having a single parent rerender every child component rather than child components rerendering themselves whenever they detect a change).

If you have specific situation which you’re trying to debug, I’m happy to help, given that your code is in the open!

---

All in all, thanks for trying out Crank and sharing your thoughts! Means a lot 🥰

[lukejagodzinski]

The app is great! I’ve been following the file system access standard closely so it’s exciting to see an app which uses it!

Glad that you like it :). I've been following this standard as well. It opens new possibilities for web dev :)

Defining CustomEvent subclasses is a great idea! I always forget to bubble custom events, and having a single file where all the custom events of your application are defined feels clean.

So maybe it could be one of a good practices mentioned in docs?

I’m not 100% on why the Table component needs to have async render props. Personally, I would probably have a single async function which turns an array of FileSystemHandle objects into the data structure you want for the table. This approach would be framework agnostic.

Yes actually, I wouldn't do it again with async component if I had current knowledge. I will probably change it into regular component. But as you suspect, there was a reason why I decided to use async. I thought like: "Ok, I have async components let's go a little bit crazy and use its full potential" :). So at the beginning I wanted to display table structure and load individual cell values asynchronously (displaying "Loading..." in the cell while waiting). Modification date, size and type are all promises so I thought that it would be nice to display something right away instead of waiting for the data to load. You know rendering initial content to the user sooner, so overall app would feel more responsive. But then, I had to implement sorting and I was like "Oh ok, I have to load async data first to do sorting", so yeah that wasn't the best approach.
So I will probably display loading indicator while waiting for the data to load and then pass resolved values to the regular component.

First, you should probably split the light/dark mode logic from the the file/directory logic. Additionally, if you’re relying on experimental browser features, you could probably also get away with implementing light/dark mode with custom css properties, though I’m not sure how that works with JSS.

Yep good point, I will split it. However, I don't know if I follow what you mean by saying "custom css properties". I'm already using css variables and prefers-color-scheme: dark media query. I don't know what else I could change there.

Second, the other file/directory provisions all seem related, and when consumed, you seem to consume multiple of these provisions at once. Therefore, I recommend putting these 4 provisions into a single object, or maybe a helper class.

Hmmm right good point. I haven't thought about it. I will try that.

Finally, provide/consume is just one way to have cross-component logic; you could also maybe move the data into a global which is shared, via one the strategies I list here.

Oh I had to miss that when reading docs. Good to know. I will probably first experiment with some other state management options. But I will keep it in mind when a good use case will emerge.

Yes, this is going to be a problem for Crank (see the discussion in #37)

Yep probably it was my error but I agree that explicit refresh is much better and cleaner. For instance, after spending most of my life programming using classes, I've recently switched (at least for some things) to functional programming and when used correctly (like pure functions) my code became so much nicer and easier to understand. And I see similar problem with refreshes in React. Sometimes, you have to deeply understand how React works under the hood to write most performant and bug-free code. I think React team made a mistake of going with synchronous approach (because it's easier to understand) while hiding a lot of logic from the user. Just by reading component's code from the top to bottom I don't really see how things will work/render. Even recently (I'm working with React for several years already) I had to debug my code line by line and do console.logs because I was frustrated that it didn't rerender in a way I wanted. Your approach with explicit refreshes is just more aligned with the functional way of thinking. If you see this.refresh() then you know it will rerender.

I have a blog post in the works about why I think this is the case

Can't wait to read it :)

If you have specific situation which you’re trying to debug, I’m happy to help, given that your code is in the open!

Thanks! :) I work on this app over weekends and if I hit a wall then I will definitely ask for help :).
I'm testing Crank.js on this app to just see if it's gonna work for bigger apps (even though it's still a small one right now, I plan to make it much more functional). I would like to use Crank.js at my work. I've mentioned to my coworkers that there is this library and asked for feedback. And, they weren't against but they had a problem with this.refresh() :) - like it's going backwards to Backbone times :). But I don't think it's a fare comparison.
So when I confirm that Crank.js can work for bigger apps I will have a bargaining chip, and I will be able to convince the team :). I'm just tired of Redux + React when working with async operations. And after spending some time with Crank.js, I missed it when I had to do another async request. It was so much more boilerplate code to just do so a simple thing.

All in all, thanks for trying out Crank and sharing your thoughts!

No problem! :) I will definitely share more thought as I spend more time with it. Thanks again for the great library!

[brainkim]

So maybe it could be one of a good practices mentioned in docs?

Yep I agree and will try to add it to the docs soon.

However, I don't know if I follow what you mean by saying "custom css properties". I'm already using css variables and prefers-color-scheme: dark media query

Ah I misunderstood the code. I assumed you were implementing all of the color scheme stuff in JS space. Maybe you could use window.matchMedia() to eliminate the need for providers here.

I thought like: "Ok, I have async components let's go a little bit crazy and use its full potential" :). So at the beginning I wanted to display table structure and load individual cell values asynchronously (displaying "Loading..." in the cell while waiting).

Yeah the sorting requirement will probably require you to wait for the entire table data anyways, but for this use-case I hope to eventually get to the point where we can coordinate components the way SuspenseList works for React by deferring the mount of certain child elements based on some API. Async mounting and unmounting of nodes is coming soon, I just need to book some time to work on it.

Your approach with explicit refreshes is just more aligned with the functional way of thinking. If you see this.refresh() then you know it will rerender.

I've mentioned to my coworkers that there is this library and asked for feedback. And, they weren't against but they had a problem with this.refresh() :) - like it's going backwards to Backbone times :). But I don't think it's a fair comparison.

Yes this is basically what I’m addressing with my draft blog post. Essentially, I’m asking why do we need data binding or reactive solutions, given that the evidence from the past five or so years suggests that most apps are pathologically over-rendered and over-executed. Crank has the unique problem of choosing to under-render rather than over-render, and this usually manifests as the UI failing to update, but I think this is a good problem insofar as it is immediately obvious and teaches you about the actual execution of your application.

I can see how people would call it “a step backwards” but I think it’s necessary. It’s tough because the true value of explicit rendering isn’t immediately obvious until you’ve spent years debugging reactive systems and tweaking them to render less for performance reasons.

[brainkim]

Closing for housekeeping purposes! Thanks for the feedback!