Skip to content

Conversation

@kristiandupont
Copy link

I've been playing with Crank again and one thing that bothered me was that I had forgotten some of the basics. Also, while LLM's are happy to write Crank code, they really lean towards a number of React-isms, which I had to tell them over and over.

So I wanted to create a plugin for ESLint. I noticed that you already have the package on NPM and in here, but it seems to never have gone anywhere, so perhaps you would like to use this as a start for an official version? It works reasonably well, though being an obvious first iteration, I am sure we will need to refine it.

Copy link
Member

@brainkim brainkim left a comment

Choose a reason for hiding this comment

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

Thank you for taking the time to do this! I left some thoughts on the rules. For the most part, this seems like an improvement to the linting situation.

There are two important footguns that I’ve been meaning to lint away in Crank:

  1. mismatched props destructuring:
function* Component({children}) {
	for ({} of this) {
		yield children;
	}
}
  1. mismatched props default value destructuring:
function* Component({children="default"}) {
  for ({children} of this) {
    // The default parameter should match
    yield children;
  }
}

These footguns are unfortunate, but necessary as far as I can tell. It seems like you’ve already implemented this in the crank-props-iterator rule but it should be a separate rule.

Adding these rules would not be a blocker for merge. I have a commit where I implemented the props matching logic: b9f0996, but never got around to the default value matching as well.

The only other nits I have is that Crank development uses Bun instead of Node (it’s nice it gives you a free unit testing library), but it’s fine.

Overall excellent work and I’m eager to get this merged and shipped ASAP! Thank you so much Kristian!

"node": ">=18.0.0"
},
"peerDependencies": {
"eslint": ">=9.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

I haven’t done the migration to ESLint 9.0 yet because it seemed like a pain in the butt. Can we support lower versions 👉👈? It’s fine if not, I’ll just upgrade I guess.

Copy link
Author

Choose a reason for hiding this comment

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

Sure

"crank/prefer-lowercase-event-props": "error",
"crank/no-react-props": "error",
"crank/no-yield-in-lifecycle-methods": "error",
"crank/require-cleanup-for-timers": "error",
Copy link
Member

Choose a reason for hiding this comment

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

The old eslint-plugin-crank had the following rules:

				// A common Crank idiom is to destructure the context iterator values
				// with an empty pattern.
				//   for (const {} of this) {}
				// TODO: Write a rule that checks for empty patterns outside of this
				// context.
				"no-empty-pattern": 0,
				"react/jsx-uses-react": 2,
				"react/jsx-uses-vars": 2,
				"react/jsx-no-undef": 2,
				"react/jsx-no-duplicate-props": 2,

The no-empty-pattern rule is a must for recommended, and I don’t mind relying on eslint-plugin-react, unless there’s a better way perhaps.

Copy link
Author

Choose a reason for hiding this comment

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

As far as I can tell, eslint-plugin-react has no rule called no-empty-pattern? Also, I am not sure I understand the value of having react/jsx-uses-react included? Will anyone require React when building Crank-based code?

Copy link
Member

Choose a reason for hiding this comment

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

"no-empty-pattern" is a core ESLint rule, not a React one. You may be right about react/jsx-uses-react.

- The Crank documentation explicitly warns against this pattern
- Helps prevent subtle bugs related to component lifecycle

#### `crank/require-cleanup-for-timers`
Copy link
Member

Choose a reason for hiding this comment

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

This rule requires the usage of this.cleanup() exclusively, but you can also put the cleanup after the props loop, or use try/finally (old code pattern):

function *Timer() {
  let seconds = 0;
  const interval = setInterval(() => this.refresh(() => {
    seconds++;
  }), 1000);
  for ({} of this) {
    yield <div>{seconds}</div>;
  }

  clearInterval(interval);
}
function *Timer() {
  let seconds = 0;
  const interval = setInterval(() => {
    seconds++;
    this.refresh();
  }, 1000);

  try {
    for ({} of this) {
      yield <div>{seconds}</div>;
    }
  } finally {
    clearInterval(interval);
  }
}

I use both the cleanup() method and after loop props in various places, so it would be nice if this was allowed.

Copy link
Author

Choose a reason for hiding this comment

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

Got it!

/**
* Indentation constants
*/
const INDENT_SIZE = 2;
Copy link
Member

Choose a reason for hiding this comment

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

Can this be inferred from the file? How do ESLint plugins typically infer indentation?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, this was something I was struggling with. It would obviously be possible to run a heuristic. I don't think I feel like doing that right away though, as it will require quite a bit more work..

Copy link
Member

Choose a reason for hiding this comment

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

Not a high priority. Other formatters will likely swoop in and fix the indentation afterwards.

A simple heuristic is probably:
Find the first line with leading whitespace.
Set that as the indent of the file.

@kristiandupont
Copy link
Author

Wow, that was a lot of fast feedback! Much appreciated! I'll address it all this evening or over the weekend if I don't have time.
A bunch of your comments about package.json and the comments and docs are actually because of AI slop. I will clean that up.

@kristiandupont
Copy link
Author

Sorry about disappearing here, something came up in my personal life. I won't have time to do any work on this right now, so if you want to create a branch that we can merge this into, you can change things around whichever way you like. Otherwise I will look into it when I have some time again.

@brainkim
Copy link
Member

brainkim commented Nov 4, 2025

No worries! I’ll pick up the torch when I can!

Currently, I’m trying to ship a server framework, a content-editable text editor library, and a way to build terminal UIs with Crank before the end of the year. But linting is important, this PR figured out a lot of the hard parts, and all I really do these days is be project manager to Claude Code anyway 😅 I'll take a look!

I hope everything is well in life and sorry to hear about the issues! Anyways, thanks for getting the ball rolling on this.

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