Skip to content

[ZEPPELIN-6372] Reimplement custom TSLint rules as ESLint rules#5279

Open
voidmatcha wants to merge 1 commit into
apache:masterfrom
voidmatcha:ZEPPELIN-6372
Open

[ZEPPELIN-6372] Reimplement custom TSLint rules as ESLint rules#5279
voidmatcha wants to merge 1 commit into
apache:masterfrom
voidmatcha:ZEPPELIN-6372

Conversation

@voidmatcha

@voidmatcha voidmatcha commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

What is this PR for?

PR #5109 removed all TSLint code from zeppelin-web-angular. This restores the two custom lint rules on the ESLint flat config as a local plugin at zeppelin-web-angular/eslint-rules:

  • constructor-params-order (ZEPPELIN-6301): constructor parameters must be ordered public, protected, private, with optional and @Optional() parameters last.
  • ordered-exports (ZEPPELIN-6325): top-level exports in public-api.ts barrels must be alphabetically ordered by module specifier.

Both are registered in eslint.config.js and enforced by the existing husky/lint-staged pre-commit hook that runs eslint --fix on staged .ts files. Logic and autofix are ported from the original TSLint rules onto the typescript-eslint AST. The rules are plain CommonJS, matching eslint.config.js.

eslint src projects reports no violations on the current tree, so no existing code changes are required.

What type of PR is it?

Improvement

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-6372

How should this be tested?

  • cd zeppelin-web-angular && npm run lint passes.
  • Add an out-of-order constructor parameter, or an unsorted export in a public-api.ts, then run eslint --fix and confirm the rule flags and reorders it.

Questions:

  • Does the license files need to update? No.
  • Is there breaking changes for older versions? No.
  • Does this needs documentation? No.

@tbonelee tbonelee left a comment

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.

Thanks for restoring these rules. The detection logic is a faithful port of the original TSLint rules (rank semantics, case-insensitive sort, file scoping all match), and the flat-config wiring is clean.

One direction-level suggestion before the inline comments: for ordered-exports, I'd lean toward a maintained community rule instead of a custom one, e.g. eslint-plugin-simple-import-sort's exports rule (or eslint-plugin-perfectionist's sort-exports). Not because of this implementation specifically: autofixers that reorder statements are just inherently tricky to get fully safe around comments and interleaved code (the inline comments show a couple of cases like that), and it's the kind of long-tail edge-case work I'd rather delegate to a dedicated upstream project than own in this repo. We don't depend on the exact legacy sort order; the intent of ZEPPELIN-6325 is just "barrels stay alphabetical", and the tree is violation-free today, so switching should only need a quick npm run lint to confirm the existing barrels still pass.

constructor-params-order is a different story: it encodes a project-specific Angular DI convention with no community equivalent, so a custom rule is exactly right there. For that one I'm asking for two changes (see inline): port the original TSLint fixer's full-trivia slicing strategy so comments survive reordering, and add a small RuleTester suite. It runs in plain Node, no new infra needed.

If there's a reason to keep the custom ordered-exports (e.g. avoiding a new dependency), that's fine too; in that case the inline fixer issues on it become blocking and it should be covered by the same test suite.

messageId: 'unordered',
fix(fixer) {
const fixText = sorted.map(entry => sourceCode.getText(entry.node).trim()).join('\n');
return fixer.replaceTextRange([first.range[0], last.range[1]], fixText);

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.

This is the kind of edge case that makes me lean toward an upstream rule here (see my summary comment). The fix replaces the whole [first.range[0], last.range[1]] span with only the tracked export texts, so anything else in that span is deleted by --fix:

export * from './zeta';
// --- widgets ---        <- deleted
export * from './alpha';

The same happens to a non-export statement between exports, or an export whose sort key is null (e.g. export {}). Current barrels are clean so this is latent, but since lint-staged runs --fix on every commit, the first future occurrence would be silent code loss. If we keep the custom rule, preserving the full text between statements (slicing from each statement's start to the next statement's start, like the TSLint getFullStart() approach) plus a regression test would cover it.


// `export default ...` and TypeScript `export = ...` are both ExportAssignment
// nodes in the TS compiler AST the original rule used, so both share this key.
if (node.type === 'ExportDefaultDeclaration' || node.type === 'TSExportAssignment') {

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.

Minor note: export default class Foo {} was a ClassDeclaration in the TSLint rule (sorted by name Foo); only export default <expr> and export = were ExportAssignment. Here every ExportDefaultDeclaration gets the fixed 'export-assignment' key. Harmless today since no barrel uses export default, but it also means we're not strictly bound to the old sort semantics, which makes switching to an upstream rule easier if we go that way.

const texts = order.map(i => sourceCode.getText().slice(ranges[i][0], ranges[i][1]).trim());
const joined = isMultiLine ? texts.join(`,\n${indent}`) : texts.join(', ');

return fixer.replaceTextRange([firstStart, lastEnd], joined);

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.

I ran the rule to check a hunch about comment handling: constructor(private b: B /* injected lazily */, public a: A) fixes to constructor(public a: A, private b: B), deleting the comment. getParamRange(b) ends at b.range[1] and getCommentsBefore(a) stops at the comma token, so the comment falls inside the replaced range but in no param's text. Relatedly, a trailing same-line comment (private a: A, // about a) travels to the next param after reordering. The original TSLint fixer handled both by slicing getFullStart()-to-getFullStart(); porting that strategy would fix both cases and also removes the need for the indent recomputation below.

}

/** @type {import('eslint').Rule.RuleModule} */
module.exports = {

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.

Could you add a RuleTester suite for this rule (valid/invalid/output cases, including the comment and multi-line scenarios noted in the other comments)? It runs under plain Node with the already-installed deps, and since this fixer rewrites source files unattended on every commit, having regression coverage for the tricky cases would make future changes to it much safer.

Comment thread zeppelin-web-angular/eslint.config.js Outdated
// Custom rule (reimplemented from TSLint, ZEPPELIN-6372): keep public-api.ts
// barrels alphabetically ordered by module specifier.
files: ['**/public-api.ts'],
plugins: { local: localRules },

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.

Since public-api.ts files also match the main **/*.ts block, this registers the local namespace twice. ESLint accepts that only while both entries are the exact same object reference, so a future refactor that breaks the identity (a spread, a second require from another path) would fail every public-api.ts lint with "Cannot redefine plugin". My suggestion: hoist the registration into a dedicated files-less config object at the top, which applies to all linted files:

{
  // register the local plugin once, for every file
  plugins: { local: localRules }
},

Then both this block and the main **/*.ts block only carry rules, each stays independent of the others' globs, and there's a single registration site.

const firstStart = ranges[0][0];
const lastEnd = ranges[params.length - 1][1];
const isMultiLine = sourceCode.getText().slice(firstStart, lastEnd).includes('\n');
const indent = ' '.repeat(params[0].loc.start.column);

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.

Nit: when the first param shares a line with constructor(, the computed indent is the first param's column (14+ spaces). Prettier re-normalizes it in the lint-staged and lint:fix pipelines, but a standalone editor eslint --fix shows it. This goes away automatically with the full-slice fixer strategy above.

Signed-off-by: YONGJAE LEE <dev.yongjaelee@gmail.com>
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