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

Add tracking of wasp version to the Wasp project #578

Closed
Martinsos opened this issue Apr 21, 2022 · 14 comments · Fixed by #786
Closed

Add tracking of wasp version to the Wasp project #578

Martinsos opened this issue Apr 21, 2022 · 14 comments · Fixed by #786
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers hacktoberfest This issue is perfect for HacktoberFest contributors hacktoberfest-accepted haskell

Comments

@Martinsos
Copy link
Member

Idea: what if we had a field in .wasp that specifies which Wasp version is used for building specific project? I think at some point we will want to have that for sure, right?
So when you want to run a Wasp project, you are sure you are not using wrong Wasp compiler version. It should be enough to just specify major.minor version -> then we know that it has to be that major version, and minor version has to be bigger then specific minor version. So basically if we have 3.2 in the file, we will allow 3.3, 3.4 and similar, but won't allow 4.0 or anything above that. This assumes you won't ever want single Wasp project to support being built with different major versions, which I think it is ok assumption for now.
We could say format has to be waspVersion: ^major.minor or waspVersion: ^0.major.minor to also support alpha/beta releases, and ^ is there to makeit more clear how wasp handles this version + to possibly make it easier to add more freedom in the future.

I would imagine this field would be required, so you can't have Wasp project for which it is not clear which Wasp version needs to be used. And if your wasp CLI doesn't match the version of the project, it would throw an error.

Field could be app.waspVersion, or maybe wasp.version (with wasp being new declaration for wasp specific config. In theory it could even be a separate file, but it seems weird to have it in separate file when we have .wasp.

@Martinsos Martinsos added enhancement New feature or request haskell labels Apr 21, 2022
@Martinsos Martinsos added the good first issue Good for newcomers label Sep 20, 2022
@Martinsos
Copy link
Member Author

Some initial direction pointers:

  1. Look into AppSpec/ and extend the AppSpec so it has waspVersion in it.
  2. In Wasp compilation step (so somewhere in Analyzer probably, or close to it), we would want to check if Wasp version matches the version in waspVersion. I am not 100% sure if we would want to do that in cli/ or in src/, but we do smth for start and then see from there.

@maksim36ua maksim36ua added the hacktoberfest This issue is perfect for HacktoberFest contributors label Oct 3, 2022
@neolight1010
Copy link
Contributor

neolight1010 commented Oct 20, 2022

Hello! I would like to work on this issue. I was already checking the code and I think I have a good idea of where to start 😄

What do you think the error message should be? I was thinking of something like "Your Wasp version (a.b.c) should match ^x.y.z"

@Martinsos
Copy link
Member Author

Hey @neolight1010 !
Sure, go for it! The error message sounds fine, maybe we can tweak it a bit in the direction of "Current Wasp project requires Wasp version ... but you are using ...".

Before you get too deep into implementation, I would maybe advise you to check with me on how you would approach it -> so if you could explain to me shortly in which pieces of code would you make changes and how it would work, we can avoid bigger corrections later. You can just write it here! And take into account what I wrote in description -> there is also a question of where we specify the version, is it in main.wasp file, or in some other file, or what.

@Martinsos Martinsos assigned neolight1010 and Martinsos and unassigned Martinsos Oct 20, 2022
@neolight1010
Copy link
Contributor

Thank you for assigning the issue to me :D

About the implementation, I think the function validateAppSpec would be a good place to validate the compiler's version, as that's where most other field-specific validation is:

validateAppSpec :: AppSpec -> [ValidationError]
validateAppSpec spec =
case validateExactlyOneAppExists spec of
Just err -> [err]
Nothing ->
-- NOTE: We check these only if App exists because they all rely on it existing.
concat
[ validateAppAuthIsSetIfAnyPageRequiresAuth spec,
validateAuthUserEntityHasCorrectFieldsIfUsernameAndPasswordAuthIsUsed spec,
validateExternalAuthEntityHasCorrectFieldsIfExternalAuthIsUsed spec,
validateDbIsPostgresIfPgBossUsed spec
]

I also thought about doing it in analyzeWaspProject or Analyzer.analyze, but that seems more related to "parsing" and converting the .wasp file's contents to AppSpec, so I don't think the validation belongs there:

analyzeWaspProject ::
Path' Abs (Dir WaspProjectDir) ->
CompileOptions ->
IO ([CompileWarning], Either (NonEmpty CompileError) AS.AppSpec)
analyzeWaspProject waspDir options = do
maybeWaspFilePath <- findWaspFile waspDir
appSpecOrAnalyzerErrors <- case maybeWaspFilePath of
Nothing -> return $ Left $ fromList ["Couldn't find a single *.wasp file."]
Just waspFilePath -> do
waspFileContent <- readFile (SP.fromAbsFile waspFilePath)
case Analyzer.analyze waspFileContent of
Left analyzeError ->
return $
Left $
fromList
[ showCompilerErrorForTerminal
(waspFilePath, waspFileContent)
(getErrorMessageAndCtx analyzeError)
]
Right decls -> do
externalCodeFiles <-
ExternalCode.readFiles (CompileOptions.externalCodeDirPath options)
maybeDotEnvServerFile <- findDotEnvServer waspDir
maybeDotEnvClientFile <- findDotEnvClient waspDir
maybeMigrationsDir <- findMigrationsDir waspDir
maybeUserDockerfileContents <- loadUserDockerfileContents waspDir
return $
Right
AS.AppSpec
{ AS.decls = decls,
AS.externalCodeFiles = externalCodeFiles,
AS.externalCodeDirPath = CompileOptions.externalCodeDirPath options,
AS.migrationsDir = maybeMigrationsDir,
AS.dotEnvServerFile = maybeDotEnvServerFile,
AS.dotEnvClientFile = maybeDotEnvClientFile,
AS.isBuild = CompileOptions.isBuild options,
AS.userDockerfileContents = maybeUserDockerfileContents
}
analyzerWarnings <- warnIfDotEnvPresent waspDir
return (analyzerWarnings, appSpecOrAnalyzerErrors)

About where the version field should go, I think it would be best to have it as wasp.version. That makes more sense to me and would allow for more compiler-specific configuration in the future.

I am still not sure about the format of the field, though. Maybe just use x.y.z for now to keep things simple, and later add the SemVer specific tokens like ^ and ~?

Let me know what you think.

@Martinsos
Copy link
Member Author

@neolight1010 nice analysis!

I am in with the idea of having smth like

wasp {
  version: "^1.2.3"
}

in AppSpec, that makes sense to me, and as you said opens some space for further configuration. If needed, we can in the future take it out, but for now this sounds ok.
We would make that required field.

I would maybe quickly check with the rest of team here though
@shayneczyzewski @sodic @matijaSos , what do you think?

As for where we would analyze it -> Yup, I think AppSpec Validation step is good!

Finally, version format: I would propose going with what I suggested in the initial comment on this issue, which is ^x.y.z or ^0.x.y.z. Then later we can possibly expand on that if needed.
Do notice that we have a SemVer module in our codebase that knows how to work very nicely with semver, so that one might come in handy!

@neolight1010
Copy link
Contributor

neolight1010 commented Oct 24, 2022

After some discussion on Discord, we agreed to use app.wasp.version instead of wasp.version, as the current implementation doesn't allow unnamed entries in the .wasp file, and adding support for that would require a lot of knowledge of the codebase.

@shayneczyzewski
Copy link
Contributor

All this sounds good @Martinsos and @neolight1010 I think doing something like app.wasp.version until we can pull it out top makes sense. 👍🏻

One other thing to keep in mind is our installer/CLI doesn't make it super clear how to install a specific version. I wonder if we should provide a snippet/info/URL for them to install the specific version they need in our error message. Over time I think our CLI will make handling multiple versions easier, but that is lacking right now. Even if this requires a bit of tweaking to the installer script, it may be worth it for near-term DX if we make this a new required field. Just a thought. Excited about this! 🥳

@neolight1010
Copy link
Contributor

Sounds good @shayneczyzewski . Where should this message lead to? I can't find any info in the docs for how to install a specific version. Maybe the GitHub Releases page?

@sodic
Copy link
Contributor

sodic commented Oct 25, 2022

Just a couple of things to add here.

Syntax considerations

I think I'd prefer the following ways of defining the version:

app Something {
    version: "^1.2.3"
    ...
}

Or maybe:

app Something {
    waspVersion: "^1.2.3"
    ...
}

Or even:

app Something {
    wasp: "^1.2.3"
    ...
}

Instead of a nested block (looks a bit excessive):

app Something {
    wasp: {
        version: "^1.2.3"
    }
    ...
}

What do you guys think?

One possible caveat

@Martinsos Keep in mind that our SemVer module does not support all Node semver version specifiers. Users might expect being able to specify something like ~1.2.3. This won't work. I don't think that's a problem for now (no one will be specifying Wasp versions anyway), it's just something to make a mental note of.

The error message and where it leads to

You can follow the style of our Node version mismatch message.

Sounds good @shayneczyzewski . Where should this message lead to? I can't find any info in the docs for how to install a specific version. Maybe the GitHub Releases page?

Ah yes, this is gonna be a problem 😅. I'll let @Martinsos answer this one.

@shayneczyzewski
Copy link
Contributor

Sounds good @shayneczyzewski . Where should this message lead to? I can't find any info in the docs for how to install a specific version. Maybe the GitHub Releases page?

I agree with @sodic this may be better answered by @Martinsos. I'm not sure if we will need to update the installer script, or if we can provide a snippet where they can easily install from a release link. Maybe we can just add a section to the docs for how to manually do so (once we test it all out) in the worst case and just point them there?

@neolight1010
Copy link
Contributor

@sodic About the specific syntax, I think app.wasp.version does look a bit verbose for now, but I think it would be useful for when more compiler options might are added.

About the SemVer module limitations, we could make it clear in the docs and in the error message that the only allowed syntax for now would be "^0.major.minor.patch", until the SemVer module is changed to allow the other tokens.

Let me know what you think :D

@Martinsos
Copy link
Member Author

Nice brainstorming happening here!

  1. app.wasp.version vs app.waspVersion vs app.wasp ...
    I don't think it matter greatly at the moment. I think choice is between app.wasp.version and app.waspVersion, others don't have great naming. Of those two, I would maybe go with the nested one, to leave some space for future configurations here. But truth is I don't know what those are, and this is probably going to change anyway, so I am good with any of these two options. I suggest @neolight1010 you pick smth and we go with it.
  2. Limitations on the version -> idea, as discussed above, is that for now we support only ^0.major.minor.patch, as @neolight1010 mentioned. Or something in that direction. But it is ok that for now we support a limited subset of versions, just to get us started, we can later expand that.
  3. Actually installing the newer version -> ah that is a tricky one as @shayneczyzewski said :D. Actually not really a tricky one, it is just that we don't have support for that yet!
    I think the best way to add support for it is to extend the installer script to support installing a specific version. Installer script is here: https://github.com/wasp-lang/get-wasp/blob/master/installer.sh . So we could extend it to take one extra argument, for example -v 0.1.2.3 which will make it install that exact version (right now it installs the latest version always). We will need to make a separate PR for this since it is in another repo. @neolight1010 you can also do that if you want, or we can leave that to someone else if it is bit too much right now.

@neolight1010
Copy link
Contributor

Cool! @Martinsos Then I think we can agree on:

  1. app.wasp.version, because I already had this one partially done 😄 I can change it to app.waspVersion if you think that's better.
  2. Supporting only ^0.major.minor.patch for now.
  3. Yes, I would love to work on that feature for the installer :D I can do it after I finish this issue, if that's ok.

Thank you, guys!

@Martinsos
Copy link
Member Author

Sounds good @neolight1010 let's do it :)!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers hacktoberfest This issue is perfect for HacktoberFest contributors hacktoberfest-accepted haskell
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants