-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
TS SDK Setup #2299
TS SDK Setup #2299
Conversation
02afc25
to
f14b617
Compare
@@ -51,6 +52,7 @@ main = withUtf8 . (`E.catch` handleInternalErrors) $ do | |||
["start"] -> Command.Call.Start | |||
["start", "db"] -> Command.Call.StartDb | |||
["clean"] -> Command.Call.Clean | |||
["ts-setup"] -> Command.Call.TsSetup |
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.
I mostly made this to help me during development, but should I leave it around for users too?
The idea is that they run wasp ts-setup
and Wasp turns a regular project into a TS SDK project.
I could rename the command too, suggestions welcome.
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.
Hm can't say, what is their experience of testing out TS SDK right now with or without this command? If it doesn't improve their experience, maybe remove it. If it helps you a lot though, maybe keep it. Long term I am guessing we won't be having this command?
We now have 3 ts configs files instead of one. So how does that work -> when they run this command, it creates those 3 config files for them? I don't see them being created though in the code. So you tell them manually to create those files?
How do we imagine this in the future where we have Wasp DSL and TS SDK at the same time -> every project will come with at least two ts config files, and then the third one can be added if they want to use TS SDK? Or maybe every proejct comes with all three since the third one (for TS SDK) doesn't hurt?
@@ -102,6 +104,7 @@ main = withUtf8 . (`E.catch` handleInternalErrors) $ do | |||
Command.Call.Start -> runCommand start | |||
Command.Call.StartDb -> runCommand Command.Start.Db.start | |||
Command.Call.Clean -> runCommand clean | |||
Command.Call.TsSetup -> runCommand tsConfigSetup |
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.
Btw, we need to come up with a proper common name for the TS SDK.
TS SDK isn't ideal because we already call our wasp
package "SDK."
I called the TS SDK package wasp-config
, which I think is ok for now.
In Haskell though, I call it tsConfig
, which is a little confusing, but changing the package's name to something that involves TS would be weird (the users would be importing a package called wasp-ts-config
in TS files).
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.
Yup I agree, TS SDK is not the best name.
Config certainly seems to be one of the keywords that makes sense. These are config files. They are wasp config files. In Wasp DSL, or in TS. Or maybe one could just call them Wasp files? Hm. Wasp spec files.
One question is, what does the import look like for it in main.wasp.mts
? Is it import { app, route } from "wasp"
? Is that maybe enough? Right now you made it from "wasp-config"
if I am correct. Maybe wasp/config
? Or wasp/spec
?
Then the question is how do we call this in general in front of users. Maybe "Wasp TS spec" or "Wasp TS config" vs "Wasp DSL spec" or "Wasp DSL config". Yeah.
And then it comes down to how we call it internally, in our code. But I think we will figure that out once we figured out the stuff above.
I don't think we have to figure this out right now, let's just go with the nomenclature you used so far for this experimental version, but then after we release it let's talk and figure out the final naming. Maybe you can add that to your TODO list for TS SDK.
("__waspProjectName__", show projectName), | ||
("__waspVersion__", defaultWaspVersionBounds) | ||
] | ||
-- TODO: We do this in all files, but not all files have all placeholders |
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.
This bothers me a little, but I guess it's no big deal?
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.
But this how it was before also, right? Yeah, that is fine then. Btw this is afunction that is called upon a file, logic that decided which files this will be called upon is not in this function, so this comment should be referencing some other peice of code I guess, that I don't see from here at the moment.
|
||
-- | Prepares the project for using Wasp's TypeScript SDK. | ||
tsConfigSetup :: Command () | ||
tsConfigSetup = do |
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.
This method is still unfinished. It's supposed to set up a project to use the TS SDK but currently only does half the work (check the TODOs).
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.
Ok so this method doesn't do anything but install npm deps? Which npm deps, just for the "wasp project", not for the generated code right? So stuff like prisma, and I guess this new wasp-config? Or how does this go, I don't have the full idea of execution flow here.
What else coudl it do: create missing ts config files, inject stuff into package json, ... ? Is this only temporary? Yeah hm this is connected to my comment above where I am trying to understand how we imagine in the future what is this setup going to look like, and also what is needed now by them, for this experimental version, to be able to use it.
-- TODO: What about doing this during Wasp start? Can we make Wasp start | ||
-- pick up whether the user wants to use the TS SDK automatically? |
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.
This is an idea for the future, not for now.
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.
I don't have a good enough grasp of the whole exeuction flow to answer this, but it does sound like we want this to happen automatically, for sure. I could probably discuss this better if I understood the pros / cons / alternatives / exec flow.
export type Decl = | ||
| { declType: 'App'; declName: string; declValue: App } | ||
| { declType: 'Page'; declName: string; declValue: Page } | ||
| { declType: 'Route'; declName: string; declValue: Route } | ||
| { declType: 'Query'; declName: string; declValue: Query } | ||
| { declType: 'Action'; declName: string; declValue: Action } | ||
| { declType: 'App'; declName: string; declValue: App } | ||
| { declType: 'Job'; declName: string; declValue: Job } | ||
| { declType: 'Api'; declName: string; declValue: Api } | ||
| { declType: 'ApiNamespace'; declName: string; declValue: ApiNamespace } | ||
| { declType: 'Crud'; declName: string; declValue: Crud } |
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.
This looks a little repetitive, but removing the repetition with a complicated generic isn't worth it IMO.
for (const [actionName, actionConfig] of actions.entries()) { | ||
decls.push({ | ||
declType: 'Action', | ||
declName: actionName, | ||
declValue: mapOperationConfig(actionConfig, parseEntityRef), | ||
}) | ||
} |
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.
These for loops are also repetitive, and I'll probably rewrite them as a generic map, and add a compile-time check making sure I've covered all the options.
But let's release it first and then deal with that.
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.
Sure, I in general just skimmed this file, let's release and then we can work on making it perfect. I am sure we can work on naming, code itself, readability (splitting it up a bit), ... .
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.
This is what we call from waspc
.
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.
This is the file that the user imports.
I could have exposed userApi
directly, but doing so through index.ts
:
- Follows the convention from
NodePacakgeFFI
. - Makes it more explicit/expected.
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.
Yeah, makes sense to me.
@@ -1,5 +1,6 @@ | |||
module Wasp.Generator.NpmInstall | |||
( installNpmDependenciesWithInstallRecord, | |||
( installProjectNpmDependencies, |
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.
I had to export this and kind of break encapsulation. @Martinsos that cool?
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.
Yeah, I guess it is fine! Maybye put it as second in the list of exported stuff.
Also, you can always make the move of creating Wasp.Generator.NpmInstall.Internal and putting it there, if you do want to make it clear it is an internal thing but still allow usage if somebody really knows what they are doing. Maybe you don't have to do this here though.
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.
Ok, did a quick shallow review, let's take of stuff that is important for release, and the rest we can do after it!
For release, what I think is most important is two comments:
- How easy it is for them to set up the project to be able to use TS SDK. Existing or new project.
- Is it ok that we pollute their package.json with mention of wasp-config package, and that we have that in our basic template.
@@ -102,6 +104,7 @@ main = withUtf8 . (`E.catch` handleInternalErrors) $ do | |||
Command.Call.Start -> runCommand start | |||
Command.Call.StartDb -> runCommand Command.Start.Db.start | |||
Command.Call.Clean -> runCommand clean | |||
Command.Call.TsSetup -> runCommand tsConfigSetup |
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.
Yup I agree, TS SDK is not the best name.
Config certainly seems to be one of the keywords that makes sense. These are config files. They are wasp config files. In Wasp DSL, or in TS. Or maybe one could just call them Wasp files? Hm. Wasp spec files.
One question is, what does the import look like for it in main.wasp.mts
? Is it import { app, route } from "wasp"
? Is that maybe enough? Right now you made it from "wasp-config"
if I am correct. Maybe wasp/config
? Or wasp/spec
?
Then the question is how do we call this in general in front of users. Maybe "Wasp TS spec" or "Wasp TS config" vs "Wasp DSL spec" or "Wasp DSL config". Yeah.
And then it comes down to how we call it internally, in our code. But I think we will figure that out once we figured out the stuff above.
I don't think we have to figure this out right now, let's just go with the nomenclature you used so far for this experimental version, but then after we release it let's talk and figure out the final naming. Maybe you can add that to your TODO list for TS SDK.
import Control.Monad.IO.Class (liftIO) | ||
import Wasp.Cli.Command (Command, require) | ||
import Wasp.Cli.Command.Require (InWaspProject (InWaspProject)) | ||
import Wasp.Generator.NpmInstall (installProjectNpmDependencies) |
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.
A bit wild, importing this from the Generator. Maybe it should not be in the Generator? or we should not be calling it here?
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.
I will add a comment.
@@ -51,6 +52,7 @@ main = withUtf8 . (`E.catch` handleInternalErrors) $ do | |||
["start"] -> Command.Call.Start | |||
["start", "db"] -> Command.Call.StartDb | |||
["clean"] -> Command.Call.Clean | |||
["ts-setup"] -> Command.Call.TsSetup |
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.
Hm can't say, what is their experience of testing out TS SDK right now with or without this command? If it doesn't improve their experience, maybe remove it. If it helps you a lot though, maybe keep it. Long term I am guessing we won't be having this command?
We now have 3 ts configs files instead of one. So how does that work -> when they run this command, it creates those 3 config files for them? I don't see them being created though in the code. So you tell them manually to create those files?
How do we imagine this in the future where we have Wasp DSL and TS SDK at the same time -> every project will come with at least two ts config files, and then the third one can be added if they want to use TS SDK? Or maybe every proejct comes with all three since the third one (for TS SDK) doesn't hurt?
|
||
-- | Prepares the project for using Wasp's TypeScript SDK. | ||
tsConfigSetup :: Command () | ||
tsConfigSetup = do |
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.
Ok so this method doesn't do anything but install npm deps? Which npm deps, just for the "wasp project", not for the generated code right? So stuff like prisma, and I guess this new wasp-config? Or how does this go, I don't have the full idea of execution flow here.
What else coudl it do: create missing ts config files, inject stuff into package json, ... ? Is this only temporary? Yeah hm this is connected to my comment above where I am trying to understand how we imagine in the future what is this setup going to look like, and also what is needed now by them, for this experimental version, to be able to use it.
-- TODO: What about doing this during Wasp start? Can we make Wasp start | ||
-- pick up whether the user wants to use the TS SDK automatically? |
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.
I don't have a good enough grasp of the whole exeuction flow to answer this, but it does sound like we want this to happen automatically, for sure. I could probably discuss this better if I understood the pros / cons / alternatives / exec flow.
@@ -1,5 +1,6 @@ | |||
module Wasp.Generator.NpmInstall | |||
( installNpmDependenciesWithInstallRecord, | |||
( installProjectNpmDependencies, |
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.
Yeah, I guess it is fine! Maybye put it as second in the list of exported stuff.
Also, you can always make the move of creating Wasp.Generator.NpmInstall.Internal and putting it there, if you do want to make it clear it is an internal thing but still allow usage if somebody really knows what they are doing. Maybe you don't have to do this here though.
-- TODO: How would it not have npm dependencies installed if we always to it in | ||
-- install_packages_to_data_dir.sh? |
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.
Hm can't remember -> does install_packages_to_data_dir.sh
also install their npm deps? And who would anyway install their npm deps in release version of wasp CLI -> they don't come packaged with wasp together with their node_modules, right?
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.
Hm do they really share any functionality? I am looking at it, and maybe I miseed sometihng, but it doesnt' seem to me like "installablePackage" has much functionality really. The only tihng it can do is returns its path, right?
So far, it was Node packages that got shipped with Wasp and you could call them from Wasp CLI Haskell code. ok.
Now you added a node package that gets shipped with Wasp, so that part is the same, but the is not that we call it from Wasp's Haskell code, instead we just have it there and it is used from somewhere else (in this case from the main.wasp.mts right?). Althought wait, we do run it, don't we? We run run.ts
? So is not also runnable in this case, in a way?
Yeah I think I would certainly split it into two modules if there is ont much shared functinality. And I would try to explain it in the comment. I am not sure what is the best name, is "installable" right. Arent they all installable? Or no because these others can't be used from user's JS code? We need to figure out there "true nature" :D so we can be sure about the names.
@@ -34,6 +36,8 @@ data Package | |||
PrismaPackage | |||
| WaspStudioPackage | |||
|
|||
data InstallablePackage = WaspConfigPackage |
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.
Yeah I think we will need some more comments, but I explained above. We need to make it clear what the difference is, what is what.
("__waspProjectName__", show projectName), | ||
("__waspVersion__", defaultWaspVersionBounds) | ||
] | ||
-- TODO: We do this in all files, but not all files have all placeholders |
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.
But this how it was before also, right? Yeah, that is fine then. Btw this is afunction that is called upon a file, logic that decided which files this will be called upon is not in this function, so this comment should be referencing some other peice of code I guess, that I don't see from here at the moment.
Notes for reviewers
You can ignore the todos (they're mostly notes for me and I'll soon be moving them somewhere else).
You can comment on them if you want, but it isn't necessary.
The
wasp ts-setup
command is unfinished (more on that below). We can discuss how functional it has to be before the release.Context
This PR is complementary to #2276.
It contains the changes that enable the user to create and work in a TS SDK project that looks like this:
Those changes include:
wasp-config
packages that servers as an API for both our users andwaspc
.wasp ts-setup
command that sets up the project to work with a config in TypeScript (TS SDK).It's just the blue parts from this graph plus the prep work necessary for making the whole thing functional. The idea is to review this PR by treating the purple parts as a black box (that part is covered in #2276).