Skip to content

Conversation

@RobinMalfait
Copy link
Member

@RobinMalfait RobinMalfait commented Oct 30, 2024

This PR improves the PostCSS migrations to make sure that we install @tailwindcss/postcss in the same bucket as tailwindcss.

If tailwindcss exists in the dependencies bucket, we install @tailwindcss/postcss in the same bucket. If tailwindcss exists in the devDependencies bucket, we install @tailwindcss/postcss in the same bucket.

This also contains an internal refactor that normalizes the package manager to make sure we can install a package to the correct bucket depending on the package manager.

@RobinMalfait RobinMalfait requested a review from a team as a code owner October 30, 2024 14:21
Comment on lines 111 to 28
return {
async add(packages: string[], location: 'dependencies' | 'devDependencies' = 'dependencies') {
let packageManager = await detectPackageManager(base)
return packageManager.add(packages, location)
},
async remove(packages: string[]) {
let packageManager = await detectPackageManager(base)
return packageManager.remove(packages)
},
Copy link
Member Author

@RobinMalfait RobinMalfait Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could return the packageManager directly, but then the API usage would look like this:

- pkg(base).add(['@tailwindcss/postcss@next'])
+ pkg(base).then(pm => pm.add(['@tailwindcss/postcss@next']))

Which isn't as nice to work with.

@RobinMalfait RobinMalfait marked this pull request as draft October 30, 2024 14:28
@RobinMalfait RobinMalfait marked this pull request as ready for review October 30, 2024 14:39
We want to install `@tailwindcss/postcss` next to `tailwindcss` (in
either `dependencies` or `devDependencies`), so we want to verify that
is the case.

We also add an explicit test where we have `tailwindcss` in
`dependencies` and one in `devDependencies`.
To make sure we are installing in the correct location (`dependencies`
vs `devDependencies`) we need to provide some flags to the package
manager.

In a perfect world the flags used by npm, pnpm, bun and yarn would be
the same but that's not the case. This abstraction allows us to use a
consistent interface where we can add and remove files and point the
location we want.
@RobinMalfait RobinMalfait force-pushed the fix/update-dependency-location branch from 57a28f9 to 49ed09a Compare October 30, 2024 14:58
Comment on lines 23 to 40
class PackageManager {
constructor(private base: string) {}

async exec(command: string) {
return exec(command, { cwd: this.base })
}

async add(
packages: string[],
location: 'dependencies' | 'devDependencies',
): ReturnType<typeof this.exec> {
throw new Error('Method not implemented.')
}

async remove(packages: string[]): ReturnType<typeof this.exec> {
throw new Error('Method not implemented.')
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we simplify all this subclassing stuff by just relying on the fact that {packageManager} add {library} -D works to save a dev dependency in npm, pnpm, yarn, and bun?

Then it's more like the old implementation which was a lot less code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guess Bun doesn't support -D, sad.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep was looking at the documentation for each and there are slight differences. But can still make it smaller and just encode the exceptions. Let me adjust.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamwathan simplified it by just encoding the exception right now: a8f54dd

RobinMalfait and others added 2 commits October 30, 2024 18:57
Instead of being explicit, let's just encode the exceptions for now.
Copy link
Contributor

@thecrypticace thecrypticace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this simplification is much nicer 👍

@adamwathan adamwathan merged commit eb54dcd into next Oct 30, 2024
1 check passed
@adamwathan adamwathan deleted the fix/update-dependency-location branch October 30, 2024 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants