Enable npm workspace for user project and generated code #3159
Conversation
Deploying wasp-docs-on-main with
|
| Latest commit: |
6a6868f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://234d88fa.wasp-docs-on-main.pages.dev |
| Branch Preview URL: | https://cprecioso-npm-workspaces.wasp-docs-on-main.pages.dev |
cprecioso
force-pushed
the
cprecioso/npm-workspaces
branch
from
b2c82e8 to
e267c5b
Compare
There was a problem hiding this comment.
This file is just stuff extracted from TsConfig.hs that I also wanted to use in PackageJson.hs.
| getOppositePackageJsonDepedencyKey :: PackageRequirement -> Maybe String | ||
| getOppositePackageJsonDepedencyKey = \case | ||
| getOppositePackageJsonDependencyKey :: PackageRequirement -> Maybe String | ||
| getOppositePackageJsonDependencyKey = \case |
There was a problem hiding this comment.
Just fixing a typo
-getOppositePackageJsonDepedencyKey
+getOppositePackageJsonDependencyKey
^ here
cprecioso
changed the title
cprecioso
changed the title
|
@infomiho ready |
| # Building the server should come after Prisma generation. | ||
| RUN cd .wasp/build/server && npm run bundle | ||
|
|
||
| # Depending on `npm`'s dependency resolution, these folders may or may not exist. |
There was a problem hiding this comment.
But the top-level folder will always exist, right? If not, we might have a bigger problem because I think other parts of Wasp rely on it existing.
|
|
||
| Remember to check out the [migration guide](https://wasp.sh/docs/migration-guides/migrate-from-0-18-to-0-19) for step-by-step documentation on how to upgrade. | ||
|
|
||
| - Wasp now requires your project's `package.json` to contain `"workspaces": [".wasp/build/*", ".wasp/out/*"]`. ([#3159](https://github.com/wasp-lang/wasp/pull/3159)) |
There was a problem hiding this comment.
I was hoping we could get away without leaking this to the user.
@cprecioso Do you maybe know how other frameworks do it? Do they use workspaces at all? If so, do they leak it in their package.json files?
There was a problem hiding this comment.
The workspaces key is our best solution for participating in dependency resolution, without changing our whole generated code architecture. It has to be at the top-level package (the user's project), there's no way around that.
Other frameworks can solve these problems by generating code into the user's project directly, and shipping their runtime dependencies into an npm-published package. The templates → libs initiatives is already taking steps in this direction, if we want to go all the way in the future
Blog-post-length explanation
The meta-framework problem
While we allow the user to believe that their project files are the entry point, a meta-framework like ours actually wants to generate their own entry point files (in our case, the server and web-app projects) that are the ones that are in charge, setup the server or the client, and choose when to call into the user-authored code.
These entry points might also have their own runtime dependencies themselves, that we want to both hide from users (they don't need to manually install them or care about them) and make them participate in the dependency tree resolution (so we participate in dependency deduplication and version resolution).
So we end up with these requirements:
- We want user-authored code to be the top-level project
- We want to import user-authored code
- We want to hide framework dependencies
- We want to participate in dep resolution
Wasp until now
- By generating code in
.wasp/out/, we satisfy (1). - We take advantage of the compiler to locate and generate precise relative paths (
../../../../src/my-entry.ts), so we satisfy (2). - By making it a package by itself (it has a
package.json), it can independently declare its own dependencies, so it satisfies (3). - not (4), since we were running the installation for the framework dependencies separately from the user project.
The requirement 4 is the one we want to address.
Other frameworks
In general what other frameworks do is:
- For (1) and (2), they generate code straight in the user's project.
They do this by either actually writing files (like TanStack Router/Start), or by generating virtual files at build time with bundler plugins (like Next)
- For (3) and (4), they ship all their possible runtime dependencies as part of an npm-published package, that exports functions for the generated code to call into.
This can be done both in the same package as their compiler (like the next package having internal imports), or in a support package (like prisma making you install @prisma/client).
This solution
Our main problem is that we're not generating the code in the user's project, but as separate packages.
-
We can't write these packages to
node_modulesbecausenpmonly resolves the dep tree when fetching the packages from the registry, and doesn't look into the writtenpackage.jsons for changes. -
We can't use
file:.wasp/out/serverbecause that doesn't resolve transitive dependencies.
So then the final option is to tell npm that these projects are actually user-authored. This signals to npm that they can change their dependency, and that it should look into their package.jsons on disk to correctly resolve the dep tree. We do that through the workspaces key.
The main constraint is that the workspaces key has to be in the top-level project. Because we want to maintain the fiction of requirement (1), we have no other way but to write this key into the user project's package.json.
Wasp in the future
For us, the same architecture as other frameworks could be reached by
- First option
- Completely adopting the templates → libs RFC
- Reducing our generated code to the minimum
- Writing this minimum-viable generated code to the user project
- Second option
- Creating a package that declares all of the possible dependencies we might need
- Moving the Wasp compiler to be a bundler plugin and emit virtual files, using dependencies from that package
There was a problem hiding this comment.
Nice write up! I certainly would like to understand this better in the future, I hope we discuss this more.
Couple of quick questions without getting too deep into it right nowm fccused on the very end of your comment, the "Wasp in the future" section:
- "Writing this minimum-viable generated code to the user project" -> what does that mean, we would actually spam their project with our code? Would it just sit there, or would they need to import it / use it somehow?
- "Moving the Wasp compiler to be a bundler plugin and emit virtual files, using dependencies from that package" -> what could this look like? Does it mean we need to rewrite it to TypeScript? Does it mean we don't have wasp CLI more? Are we limited for our big viison by being "just" a bundler plugin? Or maybe there is a way to have bundler plugin that calls Wasp CLI or something like that?
There was a problem hiding this comment.
Thanks! Just a disclaimer that those are my own opinions on where I'd take Wasp in the future, haven't talked about it with anyone, just ideas (haven't even tried them out).
- [...] what does that mean, we would actually spam their project with our code?
Yeah, maybe, if our generated code becomes small enough to not be a hassle. That's what TanStack does, and it provides you great visibility, and no need for LSP plugins or anything because the code is actually all there.
- [...] Does it mean we need to rewrite it to TypeScript? Does it mean we don't have wasp CLI more? Are we limited for our big viison by being "just" a bundler plugin?
None of this, necessarily. Implementing part of our functionality can just be an implementation detail. The bundler plugin can call out to external programs no problem (e.g. esbuild), so our current compiling model doesn't need to change dramatically. (And while there's no need to rewrite to TS, I would argue that we could compile to WASM so no need for native binaries, but even that is optional). The CLI can still exist and call out e.g. vite when needed, as an implementation detail.
But implementing the compiler as a bundler plugins could give us a lot of functionality "for free", wide hooks into modifying or adapting the user code unobtrusively; and give users a know quantity for adding functionality as e.g. vite plugins. To drive the point home, large swathes of next are implemented as Webpack plugins, but you don't need to know that at all.
There was a problem hiding this comment.
You lost me on some points here, but I think got some idea of it. Ok, future discussions!
There was a problem hiding this comment.
This is awesome. @cprecioso Do you mind copying your comment with Martin's points addressed into a notion document. Everyone on the team should read and understand this better (myself included).
There was a problem hiding this comment.
| genNpmrc :: AppSpec -> Generator [FileDraft] | ||
| genNpmrc spec | ||
| -- We only use `.npmrc` to force `npm` to error out if the Node.js version is incompatible. | ||
| -- | ||
| -- In dev mode, we already check the Node.js version ourselves before running any `npm` commands, | ||
| -- so we don't need this there. | ||
| -- | ||
| -- We do expect users to manually go into the generated directories when bundling the built ouput. | ||
| -- So we do add the `.npmrc` there to help them avoid using an incompatible Node.js version. | ||
| | AS.isBuild spec = | ||
| return | ||
| [ C.mkTmplFdWithDstAndData | ||
| (C.asTmplFile [relfile|npmrc|]) | ||
| (C.asServerFile [relfile|.npmrc|]) | ||
| Nothing | ||
| ] | ||
| | otherwise = | ||
| return [] |
There was a problem hiding this comment.
Hmm, any ideas of how we can reduce this duplication without getting too fancy?
There was a problem hiding this comment.
This code is essentially the same as the one in WebGenerator, including the comment.
There was a problem hiding this comment.
Oh I see... Mmmh I can extract maybe to the NpmWorkspaces.hs seeing as it is tangentially related? Not 100% on it tho
| toWorkspacesField = | ||
| -- While the trailing slashes do not matter, we drop them because they will be user-visible in | ||
| -- their `package.json`, and it is more customary without them. | ||
| fmap (FP.Posix.dropTrailingPathSeparator . fromRelDirP) |
There was a problem hiding this comment.
The type signature already limits the function to work only on lists, so IMO the polymorphic fmap is unnecessary and confusing.
This is a loong running discussion in Wasp btw, so don't feel obliged to change it. If anything, Martin will be happy he finally got someone on his side :)
If you're curious:
There was a problem hiding this comment.
I'll turn it to map. Reading that context, I also realized that sneaky Martin also led me to believe that "all the true Haskellers" use <> instead of ++ while I prefer the latter too. 😂
There was a problem hiding this comment.
They indeed do! You will see them using <$> and <>, like I also do these days. I was once in the past in the ++ and map camp, but I grew out of it. @infomiho and @sodic haven't yet :D.
p.s. map and ++ were made for learning really, there isn't much purpose for them when there are fmap and <>. But they had/have simpler types and are therefore more friendly for beginners (simpler error messages, no typeclasses and stuff in them).
There was a problem hiding this comment.
| -- Review when we upgrade React 19 (#2482). | ||
| ] | ||
| where | ||
| globFor dir = |
There was a problem hiding this comment.
I think we can make this function clearer with a type signature. After all, it can't just take any dir. I believe we have a special type somewhere that captures the values for build and out dir.
There was a problem hiding this comment.
... and now we can also change the argument name :)
There was a problem hiding this comment.
Plus, I advise naming functions using verbs:
makeGlobFormakeNonPosixError
I know we're all serious Haskellers pretending that "yo function is no different than a number, why would it have special treatment" (render in sponge bob mock text). But, in reality, the human mind does consider functions different than regular values, and naming them with verbs helps out.
I won't block on this though, it's a matter of style.
|
@sodic ready |
| where | ||
| validateOptional packageSpec = validatePackageJsonDependency packageJson packageSpec Optional | ||
|
|
||
| validateWorkspaces :: P.PackageJson -> [GeneratorError] |
There was a problem hiding this comment.
Nice job on adding validation!
| import qualified Data.Set as S | ||
| import Wasp.Generator.Monad (GeneratorError (GenericGeneratorError)) | ||
|
|
||
| class JsonValue a where |
There was a problem hiding this comment.
I see we are doing some logic in this file for modeling JSON and validating it.
I wonder if we could reuse some existing library for this instead of implementing it on our own?
How about Aeson itself, could it also be used for this? Maybe not, I am not sure, but worth checking.
Extremely quick googling gave me https://hackage.haskell.org/package/forma (by mark karpov, he is serious), and also https://github.com/silkapp/json-schema , maybe worth checking.
There was a problem hiding this comment.
I agree with the intention, just a note that this is not new code but extracting our old code for tsconfig validation into a common file. I'd not change this code for now to keep the PR scoped, but I'll create an issue for it (the task sounds fun actually)
cprecioso
temporarily deployed
to
fly-deploy-test
GitHub Actions
Inactive
cprecioso
temporarily deployed
to
fly-deploy-test
GitHub Actions
Inactive
|
@sodic ready |
| -- We force this to be POSIX because Windows-style paths do not accept wildcard characters. | ||
| packageWildcard = [SP.reldirP|*|] |
There was a problem hiding this comment.
This is also something that probably doesn't need to be in StrongPath. When we're constructing wildcards, we're already in string teritorry, so there's no point in pretending we're dealing with real paths, discussing Posix etc.
| { dependenciesConflictErrors = conflictErrors, | ||
| devDependenciesConflictErrors = devConflictErrors | ||
| } | ||
| conflictErrors ++ devConflictErrors |
There was a problem hiding this comment.
Do we lose something by not separating dev dependency conflicts from regular dependency conflicts?
| validateFieldValue fileName fullyQualifiedFieldName (Just expectedValue) fieldValue | ||
|
|
||
| validateFieldValue :: (Eq a, JsonValue a) => String -> FullyQualifiedFieldName -> Maybe a -> Maybe a -> [GeneratorError] | ||
| validateFieldValue fileName fullyQualifiedFieldName expectedValue actualValue = |
There was a problem hiding this comment.
When doing refactors, it's best to do it in a PR different than the one introducing a feature.
If a refactor includes both code migration and functional changes, then those two should be separated as well.
I'll leave another pointer to this great article :)
This PR includes:
- NPM workspaces changes
- A refactor of validation modules with both:
- Functional changes
- Moving code around
Since Git treats everything as new code, changes to logic are extremely easy to miss. Examples:
- I think
validateArrayFieldIncludesRequiredis new. - The
fileNameargument in this function is also new. - I think the rest of it is just moved.
This results in:
- Properly managing context and focusing attention on new changes is more difficult, so most reviewers will only do a shallow review of both the refactor and the new change.
- If the refactor needs changing, NPM workspaces have to wait.
- The review takes longer because there's more context to manage and more things to comment on. Specifically, half of my time went to the workspaces, the other half went to these three files.
I've taken a look at the refactor and have some reservations:
- A lot of the things in
Commonaren't common. Most of it is used either by one module or the other, the only common thing being theJsonValuetype class. - It seems like the refactor is half done and we could go for more reusability. Only a single function in the package.json validation does things the "TS Config" way, all others do it "The old PackageJson way" without using
JsonValue(i.e., there's little overlap between the two call stacks). I didn't go too deep, but that seems fishy. - If we're going down the JSONValue route, I think a better approach is deriving
instance JsonValue a => JsonValue [a]than doing(JsonValue [a]) => ...in a type signature. - etc.
So, since I have more reservations about the refactor than I have for the worksapces part, instead of discussing the refactor here, I suggest we move it out of this PR and into a different one? Or is it instrumental to the change?
I took a look and it doesn't seem instrumental, but I could be wrong.
I approve everything else btw (the workspaces I mean). Once this has been taken care of, you can merge without me :)
5 participants
🎉 It's here! 🎉
This PR enables npm workspaces in the users' projects, pulling the dependencies from each folder's
node_modulesto a the common top-levelnode_modules(when possible, for most packages). This was in general easier than expected, and looks like it will be mostly transparent for our users.npmwill adapt to whether the.wasp/{out,build}folders exist or not and fold them into the general dependency resolution.package.json#workspacesexists and has the correct valueNpmDependenciesto deduplicate dependencies between the user package and the framework packages.npmnow takes care of this, so we don't need it.Extra fixes that I had to do:
npxin workspaces (actually done in Correctly set dir to serve in wasp-app-runner #3168)node_modulesfolder is always created inside Docker (even if empty) so the instruction copying it doesn't fail.npmneeds every package in a workspace to have a different name. Theserverandweb-apppackages have the same name whether output to.wasp/outor.wasp/build, so I made the generation change the name based on if it's dev or build:@wasp.sh/generated-{server,client}-{build,dev}. I added an issue to improve this here Unsplit.wasp/buildand.wasp/out#3163Left over:
file:.wasp/out/sdkdependency.@types/reactthat would make the workspace installation fail. We should review when we Migrate to React 19 #2482.Steps / Criteria
package.json#workspacespackage.json's to have theworkspaceskeyworkspacesin the compilerAdaptnot neededwasp depscommand