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

Remove postinstall script #54

Merged
merged 2 commits into from
Oct 12, 2022
Merged

Remove postinstall script #54

merged 2 commits into from
Oct 12, 2022

Conversation

trevor-scheer
Copy link
Member

Having a postinstall script that runs typescript has proven to be problematic for multiple reasons. The first one being that not all projects use typescript, which forced us to use npx in our build script. The next problem to surface is that @types/node doesn't exist in the install since it's a dev dependency of this repo. I don't consider adding @types/node to the dependencies of this package to be a good solution, so I'd prefer to just unwind the problem by not having a postinstall script at all.

This just means that developers in this repo should now run the build script after cloning and installing it.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 12, 2022 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@trevor-scheer
Copy link
Member Author

Validated this change by running npm i https://pkg.csb.dev/apollographql/datasource-rest/commit/a9719ccc/@apollo/datasource-rest into an empty project.

@trevor-scheer trevor-scheer merged commit aa0fa97 into main Oct 12, 2022
@trevor-scheer trevor-scheer deleted the trevor/fix-install branch October 12, 2022 15:09
@github-actions github-actions bot mentioned this pull request Oct 12, 2022
Merged
@glasser
Copy link
Member

glasser commented Oct 12, 2022

This is basically because this came from a monorepo template and makes more sense in that context, as I think you mentioned recently, right?

trevor-scheer added a commit that referenced this pull request Oct 12, 2022
@trevor-scheer
Add prepare script
066a3e8 
In #54, I removed the `postinstall` script. Our build/publish
pipeline actually depended on the build that was a consequence of
the `postinstall` script, so this broke publishing in that the
build never occurred.

This commit adds a `prepare` script which runs the build prior to
packing for publish.
@trevor-scheer trevor-scheer mentioned this pull request Oct 12, 2022
Merged
trevor-scheer added a commit that referenced this pull request Oct 12, 2022
@trevor-scheer
Add prepack script (#57)
946d79e 
In #54, I removed the `postinstall` script. Our build/publish
pipeline actually depended on the build that was a consequence of
the `postinstall` script, so this broke publishing in that the
build never occurred.

This commit adds a `prepack` script which runs the build prior to
packing for publish.
@trevor-scheer
Copy link
Member Author

trevor-scheer commented Oct 12, 2022 •
edited
Loading

@glasser sort of, but yeah it's what we talked about before.

The postinstall is a pattern that was replicated from our monorepos into the template I created awhile back. In the monorepos, the packages published don't actually have postinstall scripts (since it all happens at the top-level package file).

So the pattern just doesn't carry over well to singular packages since it's effectful in ways that are broken for some consumers of the package. I've opened a PR to the template this package is derived from in order to stop propagating the pattern which I now consider to be bad.
apollographql/typescript-repo-template#92

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@trevor-scheer @glasser