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

fix(sdk): don't create package.json when running "prisma generate" #10542

Merged
merged 1 commit into from
Dec 7, 2021

Conversation

aqrln
Copy link
Member

@aqrln aqrln commented Dec 2, 2021

Don't create package.json when running npx prisma generate.

This logic is located on the code path where @prisma/client is going to be installed, so npm will generate the missing package.json file automatically. Our implementation, though, doesn't always finding the existing package.json file and makes it possible to accidentally create new spurious ones in the project subfolders, which, in turn, might lead to installing packages or possibly even generating the Prisma Client in the wrong location. It's better to just remove this logic and let the package manager handle this correctly.

Possibly/partially related:

Don't create `package.json` when running `npx prisma generate`.

This logic is located on the code path where `@prisma/client` is going to be installed, so npm will generate the missing `package.json` file automatically.  Our implementation, though, doesn't always finding the existing `package.json` file and makes it possible to accidentally create new spurious ones in the project subfolders, which, in turn, might lead to installing packages or possibly even generating the Prisma Client in the wrong location. It's better to just remove this logic and let the package manager handle this correctly.

Possibly/partially related:

* #9553 (comment)
* #9553 (comment)
* #10488
@aqrln aqrln requested a review from janpio December 2, 2021 19:43
@janpio
Copy link
Contributor

janpio commented Dec 2, 2021

so npm will generate the missing package.json file automatically.

Is that indeed so? Also when there is a package.json in the parent folder?

@aqrln
Copy link
Member Author

aqrln commented Dec 2, 2021

@janpio

Is that indeed so? Also when there is a package.json in the parent folder?

If there's a package.json in the current or any of the parent directories, it will use that. If there's no package.json anywhere up the tree, npm will create it in the current directory. It may be even more complex than that, especially with npm workspaces, I'm not sure about the details.

From the user perspective, you can run npm install <something> in any subdirectory no matter how many levels deep into the project and it will "just work". Or you can run it outside of an existing package, and npm will create both package.json and package-lock.json in the current directory.

@aqrln aqrln added this to the 3.7.0 milestone Dec 2, 2021
@janpio
Copy link
Contributor

janpio commented Dec 3, 2021

So right now (with the code still in place) this would prevent npm install from using an existing package.json that is more than 1 level up from where you run the command, and create a new one in the execution location instead.

And if there is no package.json at all, with this change merged, the behavior will still be the exact same as npm install (and equivalent yarn etc) automatically creates a package.json?

@aqrln
Copy link
Member Author

aqrln commented Dec 3, 2021

Generally, yes. A few notes though:

So right now (with the code still in place) this would prevent npm install from using an existing package.json that is more than 1 level up from where you run the command, and create a new one in the execution location instead.

Interestingly, it doesn't necessarily prevent the automatic installation of @prisma/client from using the correct package.json, because we run npm install @prisma/client in the directory of schema.prisma as the current working directory. But it still can break in corner cases depending on the project directories layout, and will definitely cause problems when the user runs npm install <something> in that directory or its subdirectories manually.

And if there is no package.json at all, with this change merged, the behavior will still be the exact same as npm install (and equivalent yarn etc) automatically creates a package.json?

Yes, I've just double-checked this with npm and Yarn. The only difference seems to be that the package.json they automatically create is super minimal and doesn't have anything but dependencies specified. Is there any specific reason it would be important to Prisma for the project to have a well-formed package.json file with name, version and stuff, or can we safely conclude it's not our concern as long as the package manager is fine with it?

@janpio
Copy link
Contributor

janpio commented Dec 3, 2021

Is there any specific reason it would be important to Prisma for the project to have a well-formed package.json file with name, version and stuff, or can we safely conclude it's not our concern as long as the package manager is fine with it?

Difficult statement to make. I think we modelled the created file after what npm init -y creates.

Copy link
Contributor

@janpio janpio left a comment

Choose a reason for hiding this comment

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

If we are not aware of any functionality depending on this file creation, I am in favor of removing it and waiting for something to break and then potentially fix it.

If nothing breaks, we got rid of a badly understood and completely undocumented feature with a lot of potential for damage (see issues and general discussion).

Copy link
Contributor

@Jolg42 Jolg42 left a comment

Choose a reason for hiding this comment

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

@Jolg42 Jolg42 merged commit dd30229 into main Dec 7, 2021
@Jolg42 Jolg42 deleted the integration/dont-create-package-json branch December 7, 2021 10:49
aqrln added a commit that referenced this pull request Dec 15, 2021
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