feat(tls.createSecureContext): introduce

[0hmX]

closes: #123

[AugustinMauroy]

Minor thing

[brunocroh]

You did a great job on that, thank you for your awesome contribution!

I have some comments on a few points, feel free to share your opinion if you don't agree with something.

And just one more thing: could you please check if you ran node --run pre-commit? I think there are some Biome rules that aren't applied.

[AugustinMauroy]

LGMT ! Thanks for you first contribution.
If you have any feedback about DX on this project let's me know. And if you have any feedback about codemod CLI/platform I'm going to transfer it to them

[AugustinMauroy]

fro windows failing use https://nodejs.org/api/os.html#oseol instead of \n

[AugustinMauroy]

we also have few utility you should use it to simplify

[JakobJingleheimer]

Looks like good test cases :)

Regarding cases handled, what about import expressions const { createCredentials } = await import('node:crypto') and variants like const createCredentials = (await import('node:crypto')).then((m) => m.createCredentials)?

I think that is needed in order to say the deprecation is handled.

[JakobJingleheimer]

nit: why not just write the value of newEdit into push? localEdits.push({…}) newEdits isn't used elsewhere and isn't complicated.

[JakobJingleheimer]

Not necessarily a blocker, but this file has a lot more code than I expected. There are basically 3 things that need handling, 2 of which are mostly the same:

1.1 an import statement
1.2 a require statement
2 createCredentials → createSecureContext

1.1 & 1.2 the detection is different, but the transformation is the same (the only part changing is mostly irrespective of cjs vs esm). So I would expect something like this pseudo-code:

type findDepExpressions = (ast): {
  node: ASTNode,
  type: 'import-dynamic' | 'import-static' | 'require',
};

const depExpHandlers = {
  require(n: ASTNode) {
    depExpCommonHandler(n);
    // Require-specific bits like namespacing
    return updatedNode;
  },
  'import-dynamic'(n: ASTNode) {
    depExpCommonHandler(n);
    // Dynamic-specific bits like thenable namespacing
    return updatedNode;
  },
  'import-static'(n: ASTNode) {
    depExpCommonHandler(n);
    // Static-specific bits like namespacing
    return updatedNode;
  },
};

for (const dS of findDepExpressions(ast)) edits.push(
  depExpHandlers[ds.type](ds.node),
);

[0hmX]

I am open to the idea, but this will cause a significant refactor

[AugustinMauroy]

Not necessarily a blocker, but this file has a lot more code than I expected. There are basically 3 things that need handling, 2 of which are mostly the same:

Also it's because it's doesn't use any utility.

@0hmX what do you think about using our utility to reduce the amount of code here ?

[JakobJingleheimer]

Niiiice

[AugustinMauroy]

Looks like good test cases :)

Regarding cases handled, what about import expressions const { createCredentials } = await import('node:crypto') and variants like const createCredentials = (await import('node:crypto')).then((m) => m.createCredentials)?

I think that is needed in order to say the deprecation is handled.

BTW any of our publish codemod support dynamic import

[JakobJingleheimer]

BTW any of our publish codemod support dynamic import

Sorry, is that a question or a statement? (Do you mean "all of our published codemods support dynamic import"?)

[AugustinMauroy]

BTW any of our publish codemod support dynamic import

Sorry, is that a question or a statement? (Do you mean "all of our published codemods support dynamic import"?)

It's a fact ! So do we want to support that in general ?

[0hmX]

Regarding cases handled, what about import expressions const { createCredentials } = await import('node:crypto') and variants like const createCredentials = (await import('node:crypto')).then((m) => m.createCredentials)?

I did not know we were handling this. I will add a test case for it.

[0hmX]

Okay, so I ended up rewriting the code, but it's now bigger than the previous version🫠. Yes, I am using the utilities. I added a few more test cases related to pair_pattern and import_specifier.

However, I couldn't get the logic to work for const createCredentials = (await import('node:crypto')).then((m) => m.createCredentials) because the getNodeImportCalls utility does not handle this case. I want to skip it, as adding the extra code would make the file too large.

[JakobJingleheimer]

Okay, so I ended up rewriting the code, but it's now bigger than the previous version🫠. Yes, I am using the utilities. I added a few more test cases related to pair_pattern and import_specifier.

However, I couldn't get the logic to work for const createCredentials = (await import('node:crypto')).then((m) => m.createCredentials) because the getNodeImportCalls utility does not handle this case. I want to skip it, as adding the extra code would make the file too large.

Thanks! I'm sure it was a fair amount of work 😅 (that's why I said it wasn't necessarily a blocker)

Unfortunately you force-pushed, which wipes the review history. That means the entire PR has to get reviewed again instead of just the pieces recently changed (and that massively increases the effort & time needed to review the latest changes). I've updated the repo settings to now block force-pushing to prevent this happening again (you weren't the only one to do it), but FYI the next review will take me some time to get through now because of that force-push 😬

[AugustinMauroy]

LGMT !

[JakobJingleheimer]

LGTM :)

PS Sorry it took me a while to get back to this.

[JakobJingleheimer]

nit

Suggested change

	if (spec.kind() === 'import_specifier') {
	keyNode = spec.field('name');
	aliasNode = spec.field('alias');
	} else if (spec.kind() === 'pair_pattern') {
	keyNode = spec.field('key');
	aliasNode = spec.field('value');
	} else {
	keyNode = spec;
	}
	switch (spec.kind() {
	case 'import_specifier':
	keyNode = spec.field('name');
	aliasNode = spec.field('alias');
	break;
	case 'pair_pattern':
	keyNode = spec.field('key');
	aliasNode = spec.field('value');
	break;
	default:
	keyNode = spec;
	break;
	}

[JakobJingleheimer]

Suggested change

	let aliasNode: SgNode<TypesMap, Kinds<TypesMap>> | null = null;
	let aliasNode: SgNode<TypesMap, Kinds<TypesMap>> | null = null;

[JakobJingleheimer]

nit

Suggested change

	if (!idNode || !declaration) return [];
	if (!idNode || !declaration) return [];

[JakobJingleheimer]

nit

Suggested change

	if (idNode.kind() === 'identifier')
	return handleNamespaceImport(rootNode, idNode.text(), declaration, 'require');
	if (idNode.kind() === 'object_pattern')
	return handleDestructuredImport(rootNode, idNode, declaration, 'require');
	const kind = idNode.kind();
	if (kind === 'identifier')
	return handleNamespaceImport(rootNode, idNode.text(), declaration, 'require');
	if (kind === 'object_pattern')
	return handleDestructuredImport(rootNode, idNode, declaration, 'require');

[JakobJingleheimer]

Why @ts-ignore?