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

refactor(cli): remove pkg in favour of automatically downloading Node #454

Merged
merged 98 commits into from
Dec 3, 2022

Conversation

RobertCraigie
Copy link
Owner

@RobertCraigie RobertCraigie commented Aug 1, 2022

Change Summary

This PR completely refactors how the Prisma CLI is downloaded / installed / called. We now download Node itself at runtime and use that to install the Prisma CLI and then run it directly with Node as well.

This has some significant advantages:

  • We are now no longer tied to releases of the packaged CLI
    • These were being released by Luca, who created the Go Client and is no longer at Prisma, on his own free time.
    • Only major versions were released which means the CLI couldn't be easily tested with the latest changes on the https://github.com/prisma/prisma repository
  • Prisma Studio can now be launched from the CLI
  • The TypeScript Client can now be generated from our CLI wrapper
  • We now longer have to manually download the engine binaries ourselves, that's handled transparently for us
  • A packaged version of Node no longer has to be installed for each new Prisma version
  • We now have support for ARM

However, this does not come without some concerns:

  • Size increase
    • We use https://github.com/ekalinin/nodeenv to download Node at runtime if it isn't already installed. This downloads and creates extra files that are not strictly necessary for our use case. This results in an increase from ~140MB -> ~300MB. -
    • However this size increase can be reduced by installing nodejs-bin which you can do by providing the node extra, e.g. pip install prisma[node]. This brings the total download size to be very similar to the packaged CLI.
    • This concern also doesn't apply in cases where Node is already present. It actually will greatly improve the experience in this case.

How does it work?

We now resolve a Node binary using this flow:

The first two steps in this flow can be skipped if you so desire through your pyproject.toml file or using environment variables. For example:

[tool.prisma]
# skip global node check
use_global_node = false

# skip nodejs-bin check
use_nodejs_bin = false

# change nodeenv installation directory
nodeenv_cache_dir = '~/.foo/nodeenv'

Or using the environment variables, PRISMA_USE_GLOBAL_NODE, PRISMA_USE_NODEJS_BIN and PRISMA_NODEENV_CACHE_DIR respectively.

The Prisma CLI is then installed directly from npm and ran directly using the resolved Node binary.

closes #22
closes #236
closes #370
closes #413
closes #461
closes #621

Checklist

  • Unit tests for the changes exist
  • Tests pass without significant drop in coverage
  • Documentation reflects changes where applicable
  • Test snapshots have been updated if applicable

TODO:

  • Write node tests
  • Ensure we don't use an out of date global node installation
  • Fix windows
  • Documentation
    • Document config
  • Fix TODOs in _node.py
  • Fix incorrect caching location in tests
  • Fix prisma_version not reading from environment variables
  • Audit subprocess calls
  • Test refactored fetch command
  • Check binaryTargets warning.

Agreement

By submitting this pull request, I confirm that you can use, modify, copy and redistribute this contribution, under the terms of your choice.

TODO:
- [ ] Actually resolve the current platform name
- [ ] Write node tests
- [ ] Improve query engine utils interface
- [ ] Fix windows
@codecov
Copy link

codecov bot commented Aug 1, 2022

Codecov Report

Base: 86.52% // Head: 85.99% // Decreases project coverage by -0.52% ⚠️

Coverage data is based on head (d8c5ab6) compared to base (edc9b66).
Patch coverage: 87.93% of modified lines in pull request are covered.

❗ Current head d8c5ab6 differs from pull request most recent head 74c6d14. Consider uploading reports for the commit 74c6d14 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #454      +/-   ##
==========================================
- Coverage   86.52%   85.99%   -0.53%     
==========================================
  Files         128      124       -4     
  Lines        6240     6363     +123     
  Branches     1027     1180     +153     
==========================================
+ Hits         5399     5472      +73     
- Misses        796      827      +31     
- Partials       45       64      +19     
Impacted Files Coverage Δ
databases/main.py 0.00% <0.00%> (ø)
src/prisma/binaries/__init__.py 100.00% <ø> (ø)
src/prisma/cli/commands/version.py 95.23% <ø> (ø)
src/prisma/engine/errors.py 90.00% <ø> (ø)
src/prisma/generator/generator.py 91.27% <ø> (ø)
tests/test_cli/test_cli.py 100.00% <ø> (ø)
src/prisma/cli/commands/fetch.py 55.55% <33.33%> (-44.45%) ⬇️
src/prisma/_proxy.py 91.30% <66.66%> (+0.82%) ⬆️
src/prisma/utils.py 83.07% <66.66%> (-0.80%) ⬇️
src/prisma/cli/prisma.py 86.84% <80.00%> (-7.45%) ⬇️
... and 18 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@RobertCraigie RobertCraigie marked this pull request as draft August 4, 2022 01:31
@medz
Copy link

medz commented Sep 8, 2022

Seems like a good idea. The biggest advantage I think is to save the development of the CLI part. I had this in mind when developing Prisma ORM for Dart. However, adding the Node environment in the current development environment seems to be redundant and disgusting.

@medz
Copy link

medz commented Sep 8, 2022

😊Wait for you to add this function experimentally first, if the user feedback is not disgusting to download the Node environment, I think Prisma client development in other languages can be used for reference.

@RobertCraigie
Copy link
Owner Author

However, adding the Node environment in the current development environment seems to be redundant and disgusting.

@medz Are you referring to the fact that Node will be installed into the user's environment? If so, that is not the case, Node will be downloaded to a temporary / cache directory that is not included in the user's PATH or environment setup in any way.

Prisma ORM for Dart

Wow! I'm excited to hear that Prisma has been ported to yet another language! Well done and I hope it goes well :)

Let me know if you run into any issues with your integration, I'm more than happy to help!

@RobertCraigie RobertCraigie marked this pull request as ready for review December 3, 2022 16:21
@RobertCraigie RobertCraigie merged commit bd4446d into main Dec 3, 2022
@RobertCraigie RobertCraigie deleted the refactor/remove-pkg-cli branch December 3, 2022 17:03
@RobertCraigie RobertCraigie restored the refactor/remove-pkg-cli branch December 3, 2022 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants