-
Notifications
You must be signed in to change notification settings - Fork 440
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
refactor(cli): move cli library into seperate package #2253
Conversation
Is there a list of the desired divisions? Goals for the different parts? It's important to really plan out this type of refactoring so it doesn't just end up as a mess. |
@jsteinich Please treat this more as an experiment for now :) I'm trying to see how we can move all functionality in one package that is consumed by the CLI as a first step. This iteration would not contain the goal to publish a usable API for folks, but to bundle all dependencies / actual work being done in a sub-package. This sub-package can then be used in the next step by a different package that can do the JSII interface on top of it (and ship this package as a bundled dependency), so that we don't need to use the JSII interface internally. That being said, I'm starting a second PR to envision how the JSII API shall look like, I'll share this one when that's in a better state then hello world :D |
5016a28
to
a321bcf
Compare
34676cf
to
a64edef
Compare
a64edef
to
57d84d4
Compare
57d84d4
to
8828834
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I really like the introduction of the commons
package, too ❤️
The only main thing that I'd require is to address the added ambiguity for users which is having both a @cdktf/cli
and a cdktf-cli
package. I'd prefer to rename the new package but if you want to keep the @cdktf/cli
name (which I could understand, too) the bare minimum to me would be to have some kind of warning printed when that package is installed globally accidentally:
npm i -g @cdktf/cli
...
Warning: You might be looking for cdktf-cli! @cdktf/cli is an internal package used to power the commands in the cdkt-cli
~ something like that
This allows us to build another package that wraps the CLI implementation package into a JSII compatible layer without restricting our usage to JSII compatible TS. We can simply use the @cdktf/cli package as a bundled dependency. This comes with the challenge of having smaller sub-parts like logging, errors, config being required "above the fold", which is challenging with a big bundled cli package. Therefore we also split out the commonly used types / functions into a @cdktf/commons package. This is sort of overdue since we keep adding little helpers left and right in the packages.
8828834
to
55ac262
Compare
inferring it leads to a different structure since the lib entry point is gone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: Ansgar Mertens <ansgar@hashicorp.com>
I'm going to lock this pull request because it has been closed for 30 days. This helps our maintainers find and focus on the active issues. If you've found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
This allows us to build another package that wraps the CLI implementation package
into a JSII compatible layer without restricting our usage to JSII compatible TS.
We can simply use the @cdktf/cli package as a bundled dependency.
This comes with the challenge of having smaller sub-parts like logging, errors, config being required "above the fold", which is challenging with a big bundled cli package. Therefore we also split out the commonly used types / functions into a @cdktf/commons package. This is sort of overdue since we keep adding little helpers left and right in the packages.
This is just one step on the way of having a programmatic API, it's not the real deal, but an internal dependency (so we can make it a JSII bundled dependency).