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.

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

Improve building api by first checking if api/prisma/ exists #595

Merged
merged 9 commits into from May 29, 2020

Conversation

thedavidprice
Copy link
Contributor

If an App does not include api/prisma/, then yarn rw build throws an error.

This adds a simple check to determine whether to generate the Prisma Client based on whether prisma/ exists.

@thedavidprice
Copy link
Contributor Author

@peterp I've added the "does prisma.schema exist?" check directly to generatePrismaClient(). Because it's a Listr task, it has to run something, so I've added alternate Title and echo. Thoughts?

I'm not sure if this is an improvement or not. It makes more logical sense, but I'm also now wondering if I've created some other downstream effects... thoughts?

NOTE: there's a failing test because I'll need to add a file system fixture and check for both cases. I'll do this if we decide to proceed. Next step I need to test this on Windows.

build.js

I’ve also updated this to use the new getPaths() and removed the previous "check for Prisma exists?" from here.

In the case schema.prisma does NOT exist, here's the command line output for yarn rw build:

  ✔ Skipping Prisma Client: no schema.prisma found
  ✔ Building "web"...
  ✔ Building "api"...

Do you think this works? I guess I'd prefer just having the logic in build.js to not run the generatePrismaClient tasks at all. But I'm mixed on it.

@thedavidprice thedavidprice requested a review from peterp May 27, 2020 06:17
confirmed working on Windows including support for space in path
@thedavidprice
Copy link
Contributor Author

@RobertBroersma I could use Jest guidance here if you have some time. (Beware, I'm a n00b.)

I've added some file-check logic to generate.js, which is the command to generate Prisma Client. The command now checks if the schema.prisma file exists before running. Here's the line:

const schemaExists = fs.existsSync(getPaths().api.dbSchema)

The current test is pretty basic, it confirms the Listr Task output. But of course the file check now always fails, so the test fails.

Ideally, my test will check for both cases where the file either does or does not exist. I think the best place to test the function in L13 is actually in path.ts, which I just created a new issue about. But maybe then I'm trusting fs.existsSync too much, which has already gotten me into trouble related to Windows support. So, I see a couple ways forward:

  • use fixture(s) for getPaths().api.dbSchema
  • or just handle the value of schemaExists = true|false

Recommendations for approach and Jest tools to use? And, in either case, could you point me to a Jest example I could reference?

Copy link
Contributor

@peterp peterp left a comment

Choose a reason for hiding this comment

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

🙌ship it!

packages/cli/src/commands/dbCommands/generate.js Outdated Show resolved Hide resolved
packages/cli/src/commands/dbCommands/generate.js Outdated Show resolved Hide resolved
@RobertBroersma
Copy link
Contributor

@thedavidprice I'm thinking mocking out fs would be the best way to go. The Jest docs also mention that hitting the actual file system is pretty slow and fragile.

There's an example for how to do that here, with a nice addition of being able to set which files your fake file system would contain: https://jestjs.io/docs/en/manual-mocks#examples

A manual mock file like that would mock fs for the entire test suite in the cli package. (I think this is a good thing).

I don't know what issues with Windows support you are referring to, but I'd say we should be able to rely on fs working as expected!

Hope that helps!

@thedavidprice thedavidprice merged commit 798a21e into master May 29, 2020
@thedavidprice thedavidprice deleted the dsp-change-build-check-if-prisma-dir-exists branch May 29, 2020 05:35
@thedavidprice thedavidprice added this to the next release milestone May 29, 2020
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.

None yet

3 participants