Skip to content

chore(apps/oxlint): enable oxlint#14573

Closed
camc314 wants to merge 1 commit intomainfrom
c/10-13-chore_apps_oxlint_enable_oxlint
Closed

chore(apps/oxlint): enable oxlint#14573
camc314 wants to merge 1 commit intomainfrom
c/10-13-chore_apps_oxlint_enable_oxlint

Conversation

@camc314
Copy link
Contributor

@camc314 camc314 commented Oct 13, 2025

No description provided.

@github-actions github-actions bot added A-linter Area - Linter A-cli Area - CLI labels Oct 13, 2025
@github-actions github-actions bot added the C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior label Oct 13, 2025
Copy link
Contributor Author

camc314 commented Oct 13, 2025


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@camc314 camc314 marked this pull request as ready for review October 13, 2025 21:35
Comment on lines -140 to +142
node?: Ranged | null | undefined,
beforeCount?: number | null | undefined,
afterCount?: number | null | undefined,
node?: Ranged,
beforeCount?: number,
afterCount?: number,
Copy link
Member

Choose a reason for hiding this comment

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

This may just be my lack of TS knowledge, but without | null | undefined doesn't that make things like this illegal:

sourceCode.getText(null);
sourceCode.getText(null, null, null);
sourceCode.getText(undefined);

Accepting these can be useful in cases like:

const node = someCondition ? someNode : null;
const text = sourceCode.getText(node);

Currently I think we haven't enabled "strict" TS check, but I think we want to in future (and even if we don't, presumably user may enable strict option, and we want our types to support that).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sourceCode.getText(null);
sourceCode.getText(null, null, null);
sourceCode.getText(undefined);

Yes this wouldn't be allowed, but you could just pass in undefined e.g.

sourceCode.getText(undefined)
sourceCode.getText(undefined, undefined, undefined);

This change also makes it closer to eslint's exact api

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer a closer solution to the eslint API.
But maybe you want to introduce a helper type to make it cleaner, something like:

type NullishAble<T> = T | null | undefined;

@camc314 camc314 force-pushed the c/10-13-chore_apps_oxlint_enable_oxlint branch from 32edd39 to 6f5e182 Compare October 14, 2025 13:19
@camc314 camc314 force-pushed the c/10-13-chore_napi_transform_enable_oxlint branch from 40dd640 to e8fc078 Compare October 14, 2025 13:19
@graphite-app graphite-app bot force-pushed the c/10-13-chore_napi_transform_enable_oxlint branch 2 times, most recently from 54947e0 to d07da8e Compare October 14, 2025 13:31
@graphite-app graphite-app bot changed the base branch from c/10-13-chore_napi_transform_enable_oxlint to graphite-base/14573 October 14, 2025 13:47
@github-actions github-actions bot added A-parser Area - Parser A-transformer Area - Transformer / Transpiler labels Oct 14, 2025
@graphite-app graphite-app bot force-pushed the graphite-base/14573 branch from d07da8e to 9e54002 Compare October 14, 2025 13:53
@graphite-app graphite-app bot force-pushed the c/10-13-chore_apps_oxlint_enable_oxlint branch from 6f5e182 to d838d5e Compare October 14, 2025 13:53
@graphite-app graphite-app bot changed the base branch from graphite-base/14573 to main October 14, 2025 13:54
@graphite-app graphite-app bot force-pushed the c/10-13-chore_apps_oxlint_enable_oxlint branch from d838d5e to 633d9d2 Compare October 14, 2025 13:54
@overlookmotel
Copy link
Member

Please can we hold off on this until I'm back in "office" after break? I'm unsure about the removal of | null | undefined. Wondering if we switched to "strict" mode in TS, these lint warnings would go away.

@overlookmotel
Copy link
Member

Closing in favour of #15887.

graphite-app bot pushed a commit that referenced this pull request Nov 20, 2025
2nd try at #14573. Run oxlint on oxlint. Linter inception.
Copilot AI pushed a commit that referenced this pull request Nov 21, 2025
2nd try at #14573. Run oxlint on oxlint. Linter inception.
taearls pushed a commit to taearls/oxc that referenced this pull request Dec 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-cli Area - CLI A-linter Area - Linter A-parser Area - Parser A-transformer Area - Transformer / Transpiler C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants