feat: add Fresh lint plugin

[csvn]

Adapts the two existing Fresh lint rules in Deno into it's own Lint plugin.

• https://docs.deno.com/lint/rules/fresh-handler-export
• https://docs.deno.com/lint/rules/fresh-server-event-handlers

Used as an example for #3174 for possible creating new Fresh lint rules too.

Note

Requires the AST regex selector fix that is included in Deno v2.4.5

TODO's

• Avoid duplicate rules when using fresh tag in Deno
• Add a "fresh/island-imports" rule to disallow importing fresh from Islands
  • We'll need to just check file path for **/islands/** or something, since it's not possible yet to configure the Island folder. I made Allow passing options to lint rules in deno.json denoland/deno#30295 to make this possible perhaps in the future

Potential future rules

• fresh/route-exports: Lint module exports of routes/*
  • Must have at least one export (not just config for RouteConfig)
  • Lint that the export is a valid one, e.g. handlers, default export, config, etc
  • Disallow exports that are "unknown", probably better to move to a non-route if it's shared
• fresh/island-props: Lint values that can be serialized correctly (may not work without type info)

Closes #2950

[csvn]

Perhaps a new package for linting is overkill, or could perhaps allow more drift (version mismatch between @fresh/lint and @fresh/core). The lint plugin could instead be a export on @fresh/core, e.g. @fresh/core/lint.

Given that it's unlikely for lint rules to require extra dependencies, it might be better to have the lint plugin just as a new export on @fresh/core. I feel like most projects will want the lint rules anyway, so maybe it's better to skip another dependency.

   {
     "lint": {
       "plugins": ["jsr:@fresh/core/lint"]
     }
   }

Note

It seems that it's not possible to use import maps with "lint.plugins" at the moment, so requires the jsr: prefix currently. I created denoland/deno#30297 to highlight this.

[bartlomieju]

Nice plugin architecture — the RuleModule interface, createRules helper, and NO_VISITOR pattern are all clean. Good test coverage too. A few things I noticed:

1. Test files will be published to JSR

packages/lint/deno.json has "include": ["src/**/*.ts", ...] which matches *.test.ts files. These shouldn't ship to consumers:

"publish": {
  "include": ["src/**/*.ts", ...],
  "exclude": ["src/**/*.test.ts"]
}

2. handler-export error message says "middlewares" but fires for all routes

The message is 'Fresh middlewares must be exported as "handler" but got "handlers" instead.' — but the rule triggers in any routes/ file, not just middlewares. Something like 'Fresh routes must export "handler" instead of "handlers"' would be more accurate.

3. pathSegments uses SEPARATOR_PATTERN on file:// URLs

ctx.filename appears to be a file:// URL (the tests confirm this). SEPARATOR_PATTERN from @std/path is OS-specific — on Windows it matches both \ and /, which happens to work for URLs, but it's semantically wrong. URLs always use /, so a simple .split("/") would be more correct and wouldn't need the @std/path dependency.

4. Minor things

• README rules section is TODO: COMING SOON! — should be filled in or removed before merge
• Versions are 0.0.0 placeholders (expected for a draft, just flagging)
• The server-event-handlers rule flags any attribute with a function value on custom elements (<foo-button foo={() => {}} />), not just on* — this is actually correct since you can't serialize functions to HTML in server components, but worth a comment explaining the intentional asymmetry with HTML elements

[csvn]

I think I adressed all your feedback now @bartlomieju. Rebased to address conflicts, and addressed feedback in indiviual commits. I also made a couple of commits to fix formatting and accidental change in update.ts.

• a0f56e2 addresses 1)
• 16dee50 addresses 2)
• e4a17e6 addresses 3)
• 4a3f24d & 067f94f & 8e03b67 for first and last bullet in 4)

Let me know if there's anything else you think I missed or is left to do 🙂

Edit: Nevermind, I see I must first fix my test errors!

[csvn]

Ok, only one thing that fails currently, and that's deno check on init_test.ts. Not sure how to address that though, since it fails on:

error: JSR package not found: @fresh/lint

All I can think of is to replace the current test assertion temporarily:

    // Re-enable this after `@fresh/lint` is published
    // expect(check.code).toEqual(0);
    expect(check.stderr).toBe('error: JSR package not found: @fresh/lint');

[bartlomieju]

This seems unrelated - can you remove it?

[csvn]

Fixed. Sorry, I was trying out a new editor and this was committed by accident 😅

[bartlomieju]

This should be bumped to something more sensible like 0.1.0

[bartlomieju]

I'm not sure anymore how I feel about it... It seems useful, but there are more questions than answers. How will we update these rules using Fresh updater? What if a user changes the rule on their own?

I wonder if the better mechanism would be to consume lint plugins from npm/jsr in deno lint config. Let me think about it some more.

[csvn]

I'm not sure anymore how I feel about it... It seems useful, but there are more questions than answers. How will we update these rules using Fresh updater? What if a user changes the rule on their own?

I just imagined that @fresh/update would just bump the version of the lint plugin, and all lint rules included in the plugin would run by default. The user would need to exclude rules that are built-in, as is explained in the deno docs.

// deno.json
{
  "lint": {
    "plugins": ["@fresh/lint"],
    "rules": {
      "exclude": ["fresh/excluded-rule"]
    }
  }
}

I wonder if the better mechanism would be to consume lint plugins from npm/jsr in deno lint config. Let me think about it some more.

The intent with this PR is to publish @fresh/lint, and allow deno.json to use "plugins": ["@fresh/lint"]. I was using "plugins": ["../packages/lint/src/plugin.ts"] in the www/deno.json since @fresh/lint currently does not exist yet, to avoid errors. However I'm not sure if this work or not without jsr: or not (i.e. does lint.plugins respect the import map)? But the intent was always to be usable from JSR

Sorry if I was unclear in the PR. I just felt it odd that linting rules for Fresh was tied to Deno releases, and it was harder to add lint rules for Fresh in Rust IMO. 🙂

[lunadogbot]

CI is red on every test job because init - fmt, lint, and type check project (packages/init/src/init_test.ts:228) runs deno task check against the generated project, whose deno.json now contains lint.plugins: ["jsr:@fresh/lint"] and imports["@fresh/lint"] = "jsr:@fresh/lint@^0.1.0" — a package this PR creates but that doesn't exist on JSR yet. deno lint can't resolve the plugin and the task exits 1.

Two-step landing fixes the chicken-and-egg: ship the packages/lint/ package on its own, let it publish to JSR, then a follow-up PR wires the import + lint.plugins into packages/init/src/init.ts:572,582 and packages/update/src/update.ts:146. www/deno.json can keep the local path either way.

• nit: handler-export.ts:21 selector matches export const handlers = … and export function handlers() {} but not export { handlers } re-export syntax. Uncommon in routes, but worth either covering or noting as out-of-scope. The test file doesn't exercise it either.
• nit: new Set<[filename: string, code: string]>([...]) in both *.test.ts files dedupes by array-reference (every literal is unique), so it behaves identically to a plain array but reads oddly. const okCases = [...] is clearer.
• The TODOs still listed in the PR description (dup-rule prevention, island-imports rule) suggest this is still WIP — maintainer call whether to split those out or land them together.