-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
feat(@turbo/workspaces): add new workspaces package (#3818 #3818
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
8 Ignored Deployments
|
|
Benchmark for f8cb31dClick to view benchmark
|
5eb0fc7
to
b0d9f0f
Compare
Benchmark for 63d7bc0Click to view benchmark
|
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.
Part one.
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.
Part two.
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 haven't reviewed pnpm.ts
, npm.ts
, and yarn.ts
. I'll get that on round two.
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 any strong objections to this, but couple questions:
-
Why are we making this package? I didn't see any chatter about it, so I'm curious. I'm under the impression that switching package managers is pretty easy and in user-land, so unless I'm missing some nuances, do we really want to maintain another 3,000 lines of code?
-
Nathan mentioned this inline in one place too, but for the external API, this reads a bit better to me:
# --target flag instead of --npm, et al # convert command instead of part of the package name npx @turbo/workspaces convert --target=pnpm
Also leaves open the opportunity to do more things with Workspaces in the future, which I can imagine more use cases for.
|
||
describe("clean", () => { | ||
test.each([ | ||
[true, true], |
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.
named keys in an object would be easier to read
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.
Agreed. Jest does support something similar to this, but it's pretty buggy from what I found (the name replacement wasn't working - and there's a few issues about it as well)
@mehulkar thanks for the review! Curious what you would suggest instead of a package? Just keep it in Also, out whole ethos is keeping your monorepo broken into small, distinct parts that are easily testable, buildable, and cacheable. I did at one point consider putting this in codemod, which I could still be convinced of, but I think it also makes a nice package on its own - and packages are cheap so that was my first thought. |
b0d9f0f
to
9ddd860
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.
@nathanhammond addressed, going to clean up some of the test matrix generation in a second pass, but everything else should be much cleaner.
packages/turbo-workspace-convert/__fixtures__/npm/basic/package.json
Outdated
Show resolved
Hide resolved
import fs from "fs-extra"; | ||
import { existsSync } from "fs-extra"; | ||
|
||
const pnpm = MANAGERS.pnpm; |
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.
We can, pnpm tends to be it's own special snowflake for a few cases. But I'll look into consolidating
|
||
describe("clean", () => { | ||
test.each([ | ||
[true, true], |
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.
Agreed. Jest does support something similar to this, but it's pretty buggy from what I found (the name replacement wasn't working - and there's a few issues about it as well)
9ddd860
to
86060a6
Compare
Benchmark for dc1d1f7Click to view benchmark
|
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.
Part one, 42/61.
packages/turbo-workspace-convert/__tests__/managers/npm.test.ts
Outdated
Show resolved
Hide resolved
packages/turbo-workspace-convert/__fixtures__/yarn/basic/apps/docs/package.json
Outdated
Show resolved
Hide resolved
"ui": "*" | ||
}, | ||
"devDependencies": { | ||
"tsconfig": "*" |
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.
Found our code that looks for the alternative works-but-not-documented package.json
structure:
This is something we need to fix (and we can normalize to the current state).
try { | ||
return fs.readJsonSync(packageJsonPath, "utf8"); | ||
} catch (_) { | ||
throw new ConvertError(`no "package.json" found at ${workspaceRoot}`); |
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 error can be either ENOENT
or JSON.parse
.
packages/turbo-codemod/src/commands/migrate/steps/getTurboUpgradeCommand.ts
Outdated
Show resolved
Hide resolved
86060a6
to
f2dd618
Compare
Benchmark for ac1b67c
Click to view full benchmark
|
f2dd618
to
3555287
Compare
Updated, shipping this as first version to keep the project moving and will address further feedback in followups |
3555287
to
5c13338
Compare
Benchmark for 53fd1d4Click to view benchmark
|
if (!options?.dry) { | ||
fs.rmSync(project.paths.lockfile, { force: true }); | ||
} |
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.
Don't know who caught that one, but good catch.
Codemods to convert between workspace managers. Node API will be used by
create-turbo
when cloning examples.This is also provided as a standalone CLI.