[go: nahoru, domu]
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.

Sign up for GitHub

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

Jump to bottom

feat(cli): Output version of generated Prisma Client in prisma -v #16874

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

feat(cli): Output version of generated Prisma Client in prisma -v #16874

wants to merge 15 commits into from
+120 −5

Conversation

DavidHancu
Copy link
Contributor
@DavidHancu DavidHancu commented Dec 16, 2022 •
edited
Loading

Scope

This Pull Request represents the base implementation for #11807 and adds the version of all generated Prisma Clients to the prisma -v command, which will be expanded in a nicer format later on.

Example Output

@prisma/client          : 0.0.0
Current platform        : windows
Query Engine (Node-API) : libquery-engine c9e863f2d8de6fa0c4bcd609df078ea2dde3c2b2 (at ..\..\packages\engines\query_engine-windows.dll.node)
Migration Engine        : migration-engine-cli c9e863f2d8de6fa0c4bcd609df078ea2dde3c2b2 (at ..\..\packages\engines\migration-engine-windows.exe)
Introspection Engine    : introspection-core c9e863f2d8de6fa0c4bcd609df078ea2dde3c2b2 (at ..\..\packages\engines\introspection-engine-windows.exe)
Format Binary           : prisma-fmt c9e863f2d8de6fa0c4bcd609df078ea2dde3c2b2 (at ..\..\packages\engines\prisma-fmt-windows.exe)
Format Wasm             : @prisma/prisma-fmt-wasm 4.8.0-48.c9e863f2d8de6fa0c4bcd609df078ea2dde3c2b2
Default Engines Hash    : c9e863f2d8de6fa0c4bcd609df078ea2dde3c2b2
Studio                  : 0.479.0
Generator client        : 0.0.0 (at ..\node_modules\.prisma\client)
Generator client1       : 0.0.0 (at ..\.prisma\client)
Generator client2       : 0.0.0 (at ..\another\prisma\client)

@DavidHancu DavidHancu requested review from a team and aqrln and removed request for a team December 16, 2022 20:47
@Jolg42 Jolg42 requested a review from a team December 19, 2022 13:18
@Jolg42
Copy link
Contributor
Jolg42 commented Dec 19, 2022

Hi @DavidHancu!
Thanks for this PR, after a very quick look, it looks great! 💚
We'll need someone from the Prisma Client Team to review this.
Note that with the 4.8.0 release tomorrow and holidays, it might take a bit longer than usual.

@Jolg42
Copy link
Contributor
Jolg42 commented Jan 5, 2023

After taking a second look, we are actually missing tests here in https://github.com/prisma/prisma/blob/main/packages/cli/src/__tests__/commands/Version.test.ts

We should test prisma-client-js generator with

  • no custom output
  • custom output

And in 2 different situations

  • the generated directory exist
  • the generated directory doesn't exist

Note: we might need tests using different package managers potentially, but at the moment in the codebase we do not have a setup for this. So we could start with some manual tests and see how it goes with npm / yarn 1 / pnpm at least.

I'm wondering if adding the file modified date would make sense in the output 🤔

@DavidHancu
Copy link
Contributor Author

We should test prisma-client-js generator with

  • no custom output
  • custom output

I've already tested this manually and implemented a filterDuplicates function that ensures that all generators added have different output paths.

And in 2 different situations

  • the generated directory exist
  • the generated directory doesn't exist

If the directory doesn't exist, the getGeneratedClientVersion function will return null which will exclude this generator from the final output.

If necessary, I will write the tests required for this pull request, but I will need some indication on what kind of tests need to be created and which cases should be checked.

@janpio
Copy link
Member
janpio commented Jan 5, 2023

What about adding a -v output to the end of the Ecosystem Tests runs of https://github.com/prisma/ecosystem-tests/tree/dev/packagers? That would be a good visual confirmation (potentially with no assertion, but better than nothing), @Jolg42? Then we "just" need an integration version of this PR and it would run automatically.

@DavidHancu DavidHancu mentioned this pull request Jan 5, 2023
Merged
@DavidHancu
Copy link
Contributor Author

What about adding a -v output to the end of the Ecosystem Tests runs of https://github.com/prisma/ecosystem-tests/tree/dev/packagers? That would be a good visual confirmation (potentially with no assertion, but better than nothing), @Jolg42? Then we "just" need an integration version of this PR and it would run automatically.

As suggested, I've submitted a Pull Request implementing prisma -v across all packagers: prisma/ecosystem-tests#3348.

@janpio janpio mentioned this pull request Jan 5, 2023
Closed
janpio
janpio reviewed Jan 5, 2023
View reviewed changes
packages/cli/src/Version.ts Outdated Show resolved Hide resolved
@janpio
Copy link
Member
janpio commented Jan 5, 2023

The ecosystem-tests for the packagers that @DavidHancu added worked fine, but some unrelated Docker tests now failed. It turned out there was a getConfig call that now validated env vars like DATABASE_URL used in the schema, and everywhere we call prisma -v in ecosystem-tests this seems to be the case - except in the new Docker tests. So bug identified, and fix suggested above.

Not sure this would have been caught via the suggested tests - but now that the complexity of this is clearer, we definitely need tests with some expectations that need to be met - and that is much better implemented here than in ecosystem-tests.

What might not be obvious to @DavidHancu is how to add a "generate a Client" step to these existing tests. Should this call out to the CLI itself, or do it some other way?

DavidHancu and others added 2 commits January 6, 2023 01:18
@apolanc apolanc added the PR: Feature A PR That introduces a new feature label Mar 13, 2024
@Jolg42 Jolg42 self-assigned this Mar 15, 2024
@laplab laplab self-assigned this Apr 29, 2024
jkomyno
jkomyno reviewed May 20, 2024
View reviewed changes
packages/cli/src/Version.ts
@@ -90,17 +93,38 @@ export class Version implements Command {
})

const prismaClientVersion = await getInstalledPrismaClientVersion()

const rows = [
const schemaPath = await getSchemaPath()
Copy link
Contributor

Choose a reason for hiding this comment

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

Note, getSchemaPath's return type changed recently.

Suggested change
const schemaPath = await getSchemaPath()
const { schemaPath } = await getSchemaPath()

jkomyno
jkomyno reviewed May 20, 2024
View reviewed changes
packages/cli/src/Version.ts
@@ -143,6 +166,39 @@ export class Version implements Command {
return []
}

private async getPrismaClientJSGenerators(schemaPath: string | null): Promise<
Copy link
Contributor

Choose a reason for hiding this comment

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

This function should take the { schemas } output of getSchemaPath as input, rather than calling getSchema() internally.

@laplab laplab assigned jkomyno and unassigned laplab May 20, 2024
@jkomyno jkomyno removed their assignment May 29, 2024
@CLAassistant
Copy link
CLAassistant commented Jul 8, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janpio janpio janpio left review comments

@jkomyno jkomyno jkomyno left review comments

@Jolg42 Jolg42 Awaiting requested review from Jolg42

@aqrln aqrln Awaiting requested review from aqrln

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
PR: Feature A PR That introduces a new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@DavidHancu @Jolg42 @janpio @CLAassistant @jkomyno @laplab @apolanc