Skip to content

feat: add no-deprecated-url-parse lint rule#55

Open
danielchen0 wants to merge 4 commits intomainfrom
danielchen0/no-deprecated-url-parse
Open

feat: add no-deprecated-url-parse lint rule#55
danielchen0 wants to merge 4 commits intomainfrom
danielchen0/no-deprecated-url-parse

Conversation

@danielchen0
Copy link
Copy Markdown
Collaborator

Summary

  • Adds no-deprecated-url-parse rule that detects usage of url.parse(), deprecated in Node.js
  • Suggests using new URL(input, base) instead
  • Handles default imports, namespace imports (import * as url), and named imports (import { parse }) from both url and node:url
  • 619+ deprecation warnings/week in production
  • Platform: backend only

Test plan

Scenario Expected
url.parse('...') with default import Warning flagged
url.parse('...') with import * as url from 'node:url' Warning flagged
parse('...') with import { parse } from 'url' Warning flagged
urlParse('...') with import { parse as urlParse } Warning flagged
new URL('...') No warning
url.format(...) No warning
parse() from path module No warning

@daniel-chen-1
Copy link
Copy Markdown

daniel-chen-1 Bot commented Apr 28, 2026

Pushed an auto-fix commit to unblock the failing Lint & Format check (Prettier formatting in src/rules/no-deprecated-url-parse.ts). CI should re-run shortly.

Copy link
Copy Markdown
Contributor

@lainterr lainterr Bot left a comment

Choose a reason for hiding this comment

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

Reviewed as steward. Requesting changes — the rule will produce a false positive on the recommended replacement.

Issue

The objectName === 'URL' branch flags URL.parse(...), but URL.parse is the modern static method on the WHATWG URL constructor (Node 22+, browsers) and is exactly the safe alternative we want the agent to use. Flagging it pushes the agent toward the older new URL(...) pattern that throws on invalid input — strictly worse for tolerant parsing.

From the rule:

if (
  objectName === 'url' ||
  objectName === 'URL' ||  // <-- this is the static method, not deprecated
  urlImportedNames.has(objectName ?? '')
) {
  results.push({ ...flag... });
}

This will fire on:

const result = URL.parse(maybeUrl); // valid Node 22+ / modern browser API

Proposed fix

Drop objectName === 'URL' from the condition. Only the lowercase legacy url module (and namespace/default imports of it) are deprecated.

if (
  objectName === 'url' ||
  urlImportedNames.has(objectName ?? '')
) { ... }

A test that pins this down would also be welcome:

it('does not flag URL.parse static method', () => {
  const result = lint(`const u = URL.parse('https://example.com');`);
  expect(result).toHaveLength(0);
});

Minor: the message could nudge toward either URL.parse(input) (returns null on invalid) or new URL(input, base) (throws), since both are valid replacements with different ergonomics.

Once the URL.parse exemption is in, happy to approve.

@daniel-chen-1
Copy link
Copy Markdown

daniel-chen-1 Bot commented May 1, 2026

@danielchen0 — heads up: lainterr[bot] requested changes on this PR (review submitted 2026-05-01T00:09:31Z, on the current head SHA 7712133).

Substantive concern: the rule fires on URL.parse(input), which is the modern WHATWG static method (Node 22+, browsers) — i.e. the safe alternative we want the agent to use, not the deprecated legacy url module. The reviewer suggests dropping the objectName === "URL" branch and adding a test pinning that URL.parse(...) is not flagged.

Not auto-fixable from here — the rule semantics need a real call. Pinging you to address.

— danielchen0-pr-monitor

@daniel-chen-1
Copy link
Copy Markdown

daniel-chen-1 Bot commented May 3, 2026

Auto-fix: Lint & Format CI was failing on this rebase because README.md needed prettier formatting. Pushed prettier --write README.md as a follow-up commit. CI should go green on the next run.

Rebased onto main to resolve conflicts after #51 merge.
@daniel-chen-1 daniel-chen-1 Bot force-pushed the danielchen0/no-deprecated-url-parse branch from 75269bf to d83b853 Compare May 4, 2026 16:24
@daniel-chen-1
Copy link
Copy Markdown

daniel-chen-1 Bot commented May 4, 2026

Rebased onto main after #51 merged — the conflicts were mechanical (rule-registration list, README rule table, rule-count assertion, meta.ts platform map), no logic touched. New head is d83b853, all 7 CI checks green, mergeable=true. Ready to merge when you are.

lainterr Bot and others added 2 commits May 4, 2026 18:00
Auto-resolved trivial conflicts in shared files. Resolved by Lainter (steward).
@daniel-chen-1
Copy link
Copy Markdown

daniel-chen-1 Bot commented May 4, 2026

Auto-fix: lainterr's main-merge bumped this branch's head and the README rule-table alignment drifted (prettier flagged it on the new head). Pushed prettier --write README.md as commit 7e9999a to clear the Lint & Format check.

lainterr[bot]'s CHANGES_REQUESTED on the URL.parse false-positive is still open from 05-01 (substantive — URL.parse(input) is the modern WHATWG static method, not the deprecated legacy url.parse). Pinged you about this on 05-01; flagging again since this PR is now back in the rotation. Daniel's call on whether to drop the object-form match or scope it via import-source tracking.

Auto-resolved trivial conflicts in shared files. Resolved by Lainter (steward).
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.

1 participant