feat(tls-create-secure-pair-deprecation): handle Node.js DEP0064 migration #266
Conversation
|
Missing import test case
|
…d ESM default and dynamic import cases
…uded default and dynamic ESM imports + needed refactors as PR comments) WHAT Changed: - AST-based migration - ESM/default/dynamic cases - Normalize rewritten CJS requires to `node:tls` as suggested to code section - Remove duplicated `getNodeImportStatements/getNodeRequireCalls` invocations that attempted to handle `'node:tls'` vs `'tls'`
…d dynamic import (await/then) WHAT : - ESM default import - ESM dynamic import (await assignment) - ESM dynamic import (then callback)
…cation`)' into feat(`tls-createSecurePair-deprecation`)
| ...collectDefaultImportBindings(tlsStmts), | ||
| ...collectDynamicImportIdentifiers(rootNode), | ||
| ]); | ||
| const callSites: CallSite[] = [...findCreateSecurePairCalls(rootNode)]; |
There was a problem hiding this comment.
Why spread the result of findCreateSecurePairCalls (which is already an array)?
| ...getNodeRequireCalls(root, 'tls'), | ||
| ]; | ||
| const cspBindings = unique([ | ||
| ...tlsStmts.map(s => resolveBindingPath(s as unknown as SgNode<JS>, '$.createSecurePair')).filter(Boolean) as string[], |
There was a problem hiding this comment.
All the type casting (as … as) here seems strange 🤔
| edits.push(...rewriteTlsImports(rootNode)); | ||
| if (edits.length === 0) return null; | ||
| return rootNode.commitEdits(edits); |
There was a problem hiding this comment.
nit:
| edits.push(...rewriteTlsImports(rootNode)); | |
| if (edits.length === 0) return null; | |
| return rootNode.commitEdits(edits); | |
| edits.push(...rewriteTlsImports(rootNode)); | |
| if (edits.length === 0) return null; | |
| return rootNode.commitEdits(edits); |
| const pats = [ | ||
| '$X.createSecurePair($A, $B, $C, $D)', | ||
| '$X.createSecurePair($A, $B, $C)', | ||
| '$X.createSecurePair($A, $B)', | ||
| '$X.createSecurePair($A)', | ||
| '$X.createSecurePair()', | ||
| 'createSecurePair($A, $B, $C, $D)', | ||
| 'createSecurePair($A, $B, $C)', | ||
| 'createSecurePair($A, $B)', | ||
| 'createSecurePair($A)', | ||
| 'createSecurePair()', | ||
| ]; |
There was a problem hiding this comment.
nit: pats is static and unchanged. better to move the declaration outside findCreateSecurePairCalls (ex below line 63) to avoid needlessly re-creating them on each execution of findCreateSecurePairCalls.
| for (const n of nodes) { | ||
| const x = (n as SgNode<JS>).getMatch('X')?.text(); | ||
| const binding = x ? `${x}.createSecurePair` : 'createSecurePair'; | ||
| out.push({ call: n as SgNode<JS>, binding }); |
There was a problem hiding this comment.
I think it shouldn't be necessary to cast n as SgNode<JS> 🤨
| name = p.field('key')?.text() || ''; | ||
| alias = p.field('value')?.text() || ''; |
There was a problem hiding this comment.
What does text() return on "failure"? If nullish, nit: I think better to use ?? instead of ||.
| if (!name || name === 'createSecurePair') continue; | ||
| kept.push(alias && alias !== name ? `${name}: ${alias}` : name); | ||
| } | ||
| if (kept.indexOf('TLSSocket') === -1) kept.push('TLSSocket'); |
There was a problem hiding this comment.
| if (kept.indexOf('TLSSocket') === -1) kept.push('TLSSocket'); | |
| if (kept.includes('TLSSocket')) kept.push('TLSSocket'); |
| const k = cur.kind(); | ||
| if (k === 'lexical_declaration' || k === 'variable_declaration') { |
There was a problem hiding this comment.
Single letter var names usually indicate an index/counter. We're not short on space, so I think better to name it kind.
| const k = cur.kind(); | |
| if (k === 'lexical_declaration' || k === 'variable_declaration') { | |
| const kind = cur.kind(); | |
| if (kind === 'lexical_declaration' || kind === 'variable_declaration') { |
| const pats = [ | ||
| 'const $ID = await import(\'tls\')', | ||
| 'const $ID = await import("tls")', | ||
| 'const $ID = await import(\'node:tls\')', | ||
| 'const $ID = await import("node:tls")', | ||
| 'const $ID = import(\'tls\')', | ||
| 'const $ID = import("tls")', | ||
| 'const $ID = import(\'node:tls\')', | ||
| 'const $ID = import("node:tls")', | ||
| ]; |
There was a problem hiding this comment.
I think good to extract this.
| if (id) out.push(`${id}.createSecurePair`); | ||
| } | ||
| } | ||
| return Array.from(new Set(out)); |
There was a problem hiding this comment.
Why not use your unique utility?
|
Thanks for this! |
3 participants
This pull request implements the codemod to replace deprecated
tls.createSecurePair()withTLSSocket, addressing DEP0064.tls.createSecurePair()depreciation #127Changes included:
createSecurePair(...)intonew TLSSocket(underlyingSocket, {...}), preservingtls.TLSSocketfor namespace usage.{ createSecurePair }with{ TLSSocket }in CJS/ESM (keeps namespace imports intact).pair→socketwhere applicable.This PR is part of the feature branch
feat/tls-createSecurePair-to-TLSSocket.