-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Enable npm workspace for user project and generated code #3159
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
Conversation
Deploying wasp-docs-on-main with
|
| Latest commit: |
b15f1fb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e4623771.wasp-docs-on-main.pages.dev |
| Branch Preview URL: | https://cprecioso-npm-workspaces.wasp-docs-on-main.pages.dev |
b2c82e8 to
e267c5b
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just fixing a typo
-getOppositePackageJsonDepedencyKey
+getOppositePackageJsonDependencyKey
^ here| validateDevelopmentDependencies :: P.PackageJson -> [GeneratorError] | ||
| validateDevelopmentDependencies packageJson = | ||
| concat $ | ||
| concat |
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.
Fixing a linter warning
|
@sodic ready for re-review
|
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.
All good!
I left a few comments with suggested changes on the messages. Please consider them.
I don't want to dwell on this for too long since we're refactoring it soon.
You don't need to ping me again before merging :)
| [ "Wasp requires \"workspaces\" to include", | ||
| showSet NW.workspaceGlobs, | ||
| "but is missing", | ||
| showSet $ NW.workspaceGlobs `S.difference` definedWorkspaces, | ||
| "in package.json." | ||
| ] |
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.
| [ "Wasp requires \"workspaces\" to include", | |
| showSet NW.workspaceGlobs, | |
| "but is missing", | |
| showSet $ NW.workspaceGlobs `S.difference` definedWorkspaces, | |
| "in package.json." | |
| ] | |
| [ "Wasp requires package.json \"workspaces\" to include", | |
| showSet NW.workspaceGlobs, | |
| ", but", | |
| showSet $ NW.workspaceGlobs `S.difference` definedWorkspaces, | |
| "is missing." | |
| ] |
This sounds a little more natural. But the entire message might leave users confused (because of the arrays surrounding the output).
I recommend saying:
- Wasp requires package.json "workspaces" to include: ".wasp/build", ".wasp/out". You are missing: ".wasp/build".
| dependencies :: !DependenciesMap, | ||
| devDependencies :: !DependenciesMap | ||
| devDependencies :: !DependenciesMap, | ||
| workspaces :: !(Maybe (Set String)) |
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.
How come we went with a set here?
This structure should mirror how the file works, not how we want it to work. So I believe this should be an array. If we need the set, we should introduce it in the processing.
| [ "Wasp requires \"workspaces\" to be present with the value", | ||
| showSet NW.workspaceGlobs, | ||
| "in package.json." | ||
| ] |
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.
| [ "Wasp requires \"workspaces\" to be present with the value", | |
| showSet NW.workspaceGlobs, | |
| "in package.json." | |
| ] | |
| [ "Wasp requires package.json \"workspaces\" to be present and include values:", | |
| showSet NW.workspaceGlobs, -- we do a comma separated list without the brackets. | |
| ] |
Slighly more correct and readable I think.
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.
Also, do we want to allow them to have extra workspaces? Perhaps it's simpler keeping it airtight for now: You must have this and only this.
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 initially went for not allowing it, but Miho asked me to allow it.
🎉 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