Remove postinstall script altogether
#92
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
After a couple iterations, I've decided the best approach to this is to make sure the project is never built when installed into a project. The previous workaround (#83 + #84) was insufficient because while it dropped the requirement of the host project to have typescript installed, it still required various @types/ packages to be installed, so the solution was insufficient. The hope was to preserve behavior that when this project is cloned and installed, a build would be done. I don't see a nice way to accomplish this without also having the effect of running the build script when this package is installed into another project. The `prepare` script was considered as well, but that script is called both for transitive and local installs which is what we want to avoid. The `prepack` script is necessary for CI / publish steps to ensure the build is completed before publishing to NPM. (a learning from apollographql/datasource-rest#57)
glasser
approved these changes
Oct 12, 2022
|
I wouldn't mind removing it from |
trevor-scheer
added a commit
to apollographql/apollo-server
that referenced
this pull request
Oct 19, 2022
Having a `postinstall` script provides a (sometimes) minor convenience for developers cloning the repo and updating dependencies in that `compile` is run whenever an `install` occurs. This also has indirectly led us to an implicit dependency in the publish and testing workflows that an install implies that a build is performed. The _inconvenience_ posed by this is mentioned here: apollographql/typescript-repo-template#92 (comment) ...in summary, if the repo is not in a compilable state but you want to `install` due to a dependency update, the command will exit non-zero and fail due to the `postinstall` script failure. The "solution" to this is to run `npm install --ignore-scripts` which is cumbersome. `pretest` now runs the standard `compile` script. Running `tsc` against the whole project (including test files) is unnecessary. This repo expects that all publishes occur via `changeset`. The changeset action docs recommend the publish command to include the build command: https://github.com/changesets/action#inputs. I think this could be similarly accomplished via lifecycle scripts (`prepack`, `prepublishOnly`) but they would need to be duplicated within each individual package and they would need to trigger a build at the parent's level due to interdependencies. As such, I chose the option which allowed me to keep all of that logic in top-level scripts. <!-- First, 🌠 thank you 🌠 for taking the time to consider a contribution to Apollo! Here are some important details to follow: * ⏰ Your time is important To save your precious time, if the contribution you are making will take more than an hour, please make sure it has been discussed in an issue first. This is especially true for feature requests! * 💡 Features Feature requests can be created and discussed within a GitHub Issue. Be sure to search for existing feature requests (and related issues!) prior to opening a new request. If an existing issue covers the need, please upvote that issue by using the 👍 emote, rather than opening a new issue. * 🔌 Integrations Apollo Server has many web-framework integrations including Express, Koa, Hapi and more. When adding a new feature, or fixing a bug, please take a peak and see if other integrations are also affected. In most cases, the fix can be applied to the other frameworks as well. Please note that, since new web-frameworks have a high maintenance cost, pull-requests for new web-frameworks should be discussed with a project maintainer first. * 🕷 Bug fixes These can be created and discussed in this repository. When fixing a bug, please _try_ to add a test which verifies the fix. If you cannot, you should still submit the PR but we may still ask you (and help you!) to create a test. * 📖 Contribution guidelines Follow https://github.com/apollographql/apollo-server/blob/main/CONTRIBUTING.md when submitting a pull request. Make sure existing tests still pass, and add tests for all new behavior. * ✏️ Explain your pull request Describe the big picture of your changes here to communicate to what your pull request is meant to accomplish. Provide 🔗 links 🔗 to associated issues! We hope you will find this to be a positive experience! Open source contribution can be intimidating and we hope to alleviate that pain as much as possible. Without following these guidelines, you may be missing context that can help you succeed with your contribution, which is why we encourage discussion first. Ultimately, there is no guarantee that we will be able to merge your pull-request, but by following these guidelines we can try to avoid disappointment. -->
trevor-scheer
added a commit
to apollo-server-integrations/apollo-server-integration-koa
that referenced
this pull request
Nov 1, 2022
The `postinstall` hook provides a minor convenience for developers cloning the repo and developing within it, but only as far as running the build for you whenever you install. The drawback to using `postinstall` is that compilation runs whenever this package is installed into another repo, which is totally unnecessary and often (for JS users) results in failures when tsc or types packages aren't installed within the project. See also: apollographql/typescript-repo-template#84 apollographql/typescript-repo-template#92 Fixes #40
trevor-scheer
added a commit
to apollo-server-integrations/apollo-server-integration-koa
that referenced
this pull request
Nov 1, 2022
The `postinstall` hook provides a minor convenience for developers cloning the repo and developing within it, but only as far as running the build for you whenever you install. The drawback to using `postinstall` is that compilation runs whenever this package is installed into another repo, which is totally unnecessary and often (for JS users) results in failures when tsc or types packages aren't installed within the project. See also: apollographql/typescript-repo-template#84 apollographql/typescript-repo-template#92 Fixes #40
trevor-scheer
added a commit
to apollo-server-integrations/apollo-server-integration-hapi
that referenced
this pull request
Nov 10, 2022
For additional commentary, see: apollographql/typescript-repo-template#92
This was referenced Nov 10, 2022
trevor-scheer
added a commit
to apollo-server-integrations/apollo-server-integration-aws-lambda
that referenced
this pull request
Jan 19, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After a couple iterations, I've decided the best approach to this is to make sure the project is never built when installed into a project.
The
postinstallscript is a small convenience for local development such that a build is performed after cloning and runningnpm installin this repo. Relatedly, our publishing steps also depended on the build that happened due to the execution of this script.The previous workaround (#83 + #84) was insufficient because while it dropped the requirement of the host project to have
typescriptinstalled, it still required various@types/packages to be installed, so the solution was insufficient and still caused errors for non-TS users installing this package (really, packages that scaffold off this template).The hope was to preserve behavior that when this project is cloned and installed, a build would be performed. I don't see a nice way to accomplish this without also having the effect of running the
buildscript when this package is installed into another project.The
preparescript was considered as well, but that script is called both for transitive and local installs which is what we want to avoid.The
prepackscript is necessary for CI / publish steps to ensure the build is completed before publishing to NPM. (a learning from apollographql/datasource-rest#57)