Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow providing custom runner per transform file thru codeshiftConfig #97

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kiprasmel
Copy link
Contributor

@kiprasmel kiprasmel commented Jan 24, 2022

depends on:

example on how it could be used:

fixes a lot of cases for us:

  1. we have a postcss codemod that we want to run,
    while still utilizing the @codeshift/cli.

though, i don't know if these changes will work
if we're using a remote package, will they?

  1. we'll want to do some global pre-processing
    on files before running our codemod.

though, there's still no way to provide the codemod
as a function instead of an import path to jscodeshift,
which will force us to do dependency injection
instead of just passing the pre-processed results
as an argument to a function.

this is where the considerations to fork jscodeshift
come into play again:

@kiprasmel kiprasmel changed the title feat: allow providing custom runner per transform file thru CodeshiftConfig feat: allow providing custom runner per transform file thru codeshiftConfig Jan 24, 2022
@kiprasmel kiprasmel force-pushed the feat/custom-runner branch 6 times, most recently from c1905e9 to ceed347 Compare January 26, 2022 10:56
@kiprasmel
Copy link
Contributor Author

would be great to review if possible @danieldelcore 🙏

@kiprasmel kiprasmel force-pushed the feat/custom-runner branch 5 times, most recently from e063471 to 1013148 Compare January 26, 2022 20:03
@danieldelcore
Copy link
Contributor

danieldelcore commented Jan 28, 2022

Really interesting, could you give me some more background as to how this all fits together? As I understand it, instead of only being able to modify js / ts files you want to be able to provide a custom runner to begin to support other types of files like css?

Can you share an example codeshift.config with me?

Also i've just merged some fixes/improvements, you might want to rebase 😄

@kiprasmel
Copy link
Contributor Author

kiprasmel commented Jan 28, 2022

@danieldelcore hey! yes, correct, we want to have independance from upstream (you) so that we don't need to wait to get some custom runner merged (sometimes it could be very specific even, and you might not even need it merged, and that's fine), while still utilizing your CLI w/o publishing our own, and this unlocks the door.

this could even be used if e.g. you want to do more steps before/after the codemod run, e.g. doing some pre-processing, or idk updating some npm dependencies -- you can basically add any code there. the value is that we don't need to create wrapper CLIs around it, and just use the @codeshift/cli as it is.

re: updates -- coolio, rebased smoothly.

an example I already linked to -- https://github.com/pipedrive/CodeshiftCommunity/pull/22

and you can read on how to use the codemods (we have a simple wrapper run.js which by now the only unique functionality it has is shorthands.json; run.js is also the reason why i asked to change the config from .ts to .js! though, some community/ packages here still use the non-cjs export default instead of module.exports = . and it's also the reason why we got rid of require.resolve or path.resolve inside of codeshift.config.js, and would recommend applying this to packages here as well. (the config becomes almost like a json file at this point)):

you can try running the demo files. i just added a new one for scss:

the only concern is -- will this work if we publish our npm package? i haven't had a chance to try that; i need to first extract into a separate repository. but ideally we'd figure this out here before i do that.

and maybe a slight change i'd do is renaming codeshiftConfig to just codemodsConfig or codemodConfig or both. goes in line with the same idea that we should have an npm package codemods-cli that you could npx to, instead of @codeshift/cli.

wdyt? would be very useful.

in the issue you linked [1] from jscodeshift, i think you forgot the `path.resolve` for non-http(s) imports (not even sure how they'd work tbh).

i tried running a local transform and it didn't work, but with this fix it did regardless of where i called the `packages/cli/bin/codeshift-cli.js` from. i think you might've been using the packages and not the --transforms flag so i understand why you could've missed it 😅

tbh there are more improvements to make w/ the local transforms (see [2]) but this is the first step :D

[1] facebook/jscodeshift#398
[2] hypermod-io#48 (comment)

Signed-off-by: Kipras Melnikovas <[email protected]>
@kiprasmel kiprasmel force-pushed the feat/custom-runner branch from c8a7ddf to 46ad651 Compare July 8, 2022 15:17
…Config

depends on:
- hypermod-io#63

example on how it could be used:
- https://github.com/pipedrive/CodeshiftCommunity/pull/22
    - though note we might refactor into separate PRs, idk,
      preferably would use directly from upstream (you).

fixes a lot of cases for us:

1. we have a postcss codemod that we want to run,
while still utilizing the @codeshift/cli.

though, i don't know if these changes will work
if we're using a remote package, will they?

2. we'll want to do some global pre-processing
on files before running our codemod.

though, there's still no way to provide the codemod
as a __function__ instead of an __import path__ to jscodeshift,
which will force us to do dependency injection
instead of just passing the pre-processed results
as an argument to a function.

this is where the considerations to fork jscodeshift
come into play again:
- hypermod-io#67

Signed-off-by: Kipras Melnikovas <[email protected]>
@kiprasmel kiprasmel force-pushed the feat/custom-runner branch from 46ad651 to b25c05e Compare July 8, 2022 15:32
@danieldelcore danieldelcore force-pushed the main branch 2 times, most recently from facba25 to 5248c0e Compare July 15, 2024 01:25
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.

2 participants