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

[brunocroh]

I do not have a Windows machine to run the test, but I have updated as suggested, so could we give this another run cc: @AugustinMauroy @brunocroh

We have a CI to test these changes on Windows as well. Soon, a maintainer will enable it to run on your PR.

[JakobJingleheimer]

Thanks for this! I'll try to review the rest tonight.

[JakobJingleheimer]

I think the name should be createCredentials-to-createSecureContext

[brunocroh]

Hey Jacob, I had a quick thought about naming:

In the other codemod-recipes, we’ve been using kebab case and lowercase, with the folder name and package.json matching.

I assumed this was because npm packages typically follow this convention and are case-insensitive. I was just thinking it might help avoid potential issues with OS differences (case-sensitive vs case-insensitive) and make it a bit easier for users to search for or use the codemod.

I’m still pretty new to the project, so there’s a good chance I might be missing some context, if this has already been discussed or decided, feel free to let me know. 🙂 Cheers!

I test it now, and codemod usage is case-sensitive

[JakobJingleheimer]

oh, sure whatever. all minuscule is fine if something requires it. it was more about the $old-to-$new :)

[JakobJingleheimer]

Nit: these can be declared in the root scope to avoid re-declaring them each time (they're constant and immutable).

[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 😬