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

chore (cli): extract cli to separate package #1317

Merged
merged 22 commits into from
Jun 25, 2024

Conversation

kevin-dp
Copy link
Contributor

@kevin-dp kevin-dp commented Jun 3, 2024

This PR extracts the CLI out of the electric-sql package and into its own @electric-sql/cli package.

TODO:

  • We may want to add .js extension to every import to avoid having to run the fix-imports.js script.

Copy link

linear bot commented Jun 3, 2024

@kevin-dp kevin-dp marked this pull request as draft June 3, 2024 13:20
@kevin-dp kevin-dp force-pushed the kevindp/vax-1887-extract-cli branch 3 times, most recently from 6e9670b to bd42a80 Compare June 3, 2024 13:36
@kevin-dp kevin-dp marked this pull request as ready for review June 4, 2024 08:08
Copy link
Contributor

@msfstef msfstef left a comment

Choose a reason for hiding this comment

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

Nice work, looks good! Left some minor comments mostly relating to having some way to share configs (eslint, prettier, tsconfig) and utilities (like fix-imports.js) which can be addressed in a separate PR

components/cli/.eslintrc.cjs Outdated Show resolved Hide resolved
@@ -0,0 +1,117 @@
import * as fs from 'fs'
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: perhaps we also want to factor this out as a common utility between packages (although really I'd like us to find a way to not need this altogether haha) - maybe as an unpublished pnpm package?

Copy link
Contributor

@samwillis samwillis Jun 4, 2024

Choose a reason for hiding this comment

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

This landed in #659 and was also discussed in the abandoned #642

#642 would have gone with the "use .js imports everywhere route". Personally I would go that way with any new package (it's what I've done with PGlite) but there was push back and we landed on this somewhat ugly hack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree with Sam, i would prefer the use .js imports route. I might update all imports in the CLI package after rebasing and including the changes made to the CLI in concurrent PRs.

components/cli/tsconfig.json Show resolved Hide resolved
@kevin-dp kevin-dp force-pushed the kevindp/vax-1887-extract-cli branch from 3fdaed6 to 8a7bfde Compare June 4, 2024 15:34
@kevin-dp kevin-dp force-pushed the kevindp/vax-1887-extract-cli branch 3 times, most recently from 5f1b1a4 to 85716b2 Compare June 6, 2024 10:19
@kevin-dp kevin-dp force-pushed the kevindp/vax-1887-extract-cli branch from d2f8080 to 2ebe05b Compare June 24, 2024 11:45
@kevin-dp kevin-dp merged commit cb19c58 into main Jun 25, 2024
28 checks passed
@kevin-dp kevin-dp deleted the kevindp/vax-1887-extract-cli branch June 25, 2024 13:45
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.

None yet

3 participants