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

Include Node Binaries Rather Than Fighting With NPM #766

Merged
merged 35 commits into from
Sep 24, 2019

Conversation

designatednerd
Copy link
Contributor

The need to install things via npm in order to run code generation is unnecessarily plunging iOS developers into Node dependency hell, a place nobody wants to go.

This PR changes things up by including pre-built binaries of both our apollo CLI and of all its dependencies (including Node itself). This allows a few things:

  • Devs don't have to care about whether the version of the CLI they're using is compatible with the SDK, since they're tied together.
  • Devs don't have to worry about some kind of accidental breaking change being committed to tooling breaking their build.
  • Lays the foundation for moving the actual generation of code out of Node and into Swift (for now, we're keeping the parsing in Node).

This of course comes with a few trade-offs:

  • These binaries are not small, so it will make downloading the SDK take more time. However, since they are not included in the framework, it does not affect the size of the framework itself.
  • Swift Package Manager and Carthage will need to depend on the checked-out version of the SDK rather than something bundled within the framework. This is more of an issue for Carthage since some Carthage users don't run carthage checkout on a regular basis, but it also vastly simplifies the build script, and in turn the number of places where things can go wrong.

I'm opening this as a draft PR - I'd love some feedback, particularly from Carthage users, on how migrating from the current setup to migrating to this setup would affect your project. You should be able to point to the branch this PR is opened off of in order to see the updated documentation instructions and get the binaries.

@designatednerd designatednerd added this to the 0.16.0 milestone Sep 16, 2019
@designatednerd
Copy link
Contributor Author

OK - gonna leave this up for another day for comment - if we don't hear back from folks by Wednesday the 18th at 2pm US-Central time, I'm gonna merge it.

@koenpunt
Copy link
Contributor

koenpunt commented Sep 18, 2019

As much as I like to see the installation to be simplified for non-js users, I think it's a really bad idea to add this much javascript files to a swift project.

As I mentioned in a different thread; it should be possible to bundle all these files in a single binary file, and that binary should then be downloaded separately of the library, but this could of course be handled by a script in the library.

@koenpunt
Copy link
Contributor

koenpunt commented Sep 18, 2019

But if you really want to do this, you should at least remove the typescript declaration files (.d.ts) and sourcemap files (.map) since those are not necessary to run the code.

@koenpunt
Copy link
Contributor

With minimal effort I was able to create a single apollo binary (127M);

Install pkg (https://github.com/zeit/pkg)

npm install -g pkg

Navigate to the apollo directory;

cd scripts/apollo

Update bin in package.json;

"bin": "bin/run",

Run pkg;

pkg -t node10-macos .

@designatednerd
Copy link
Contributor Author

@koenpunt This is a fair point. I'll see what i can do about getting just a raw binary rather than the tarball that has some binary and some stuff zipped

@koenpunt
Copy link
Contributor

koenpunt commented Sep 20, 2019

@abernix thanks for weighing in on this matter.

Some brief responses on your points;

  1. Don't pack the entirety of the module into this repo

Well yes that, but mainly because it's unrelated source code. It think it would even make the repo identify itself on GitHub as being javascript.

  1. Debuggability

I wasn't necessarily saying we have to use pkg (there are other tools with similar functionality), but since I recently discovered that the firebase cli (by Google) is also a single binary node application, I figured this might be a good solution.

  1. Simplicity of packaging

Yeah I saw that, and I looked at their documentation first, hoping to find a single binary compilation command or something. But there isn't.

  1. Longevity and stability

Same as 1.

  1. Repo bloat / offline support

I think it might be good to keep it locked outside

I agree on this 100%. Maybe not for the same reasons, but adding binaries to the repo will increase its absolute size. And every time the CLI is updated, the absolute repo size will increase by the full archive size (currently 21mb), since there are not going to be any diffs between those archives. So depending on how often you intend to update the CLI, this could get out of hand pretty quickly.

And about offline support; I'd say that we can inform users about the additional step(s) for offline support in the setup instructions. For CocoaPods users this would be less of a concern, since there we could set up an after-install hook.

  1. Lock-step versioning

Ruby (or actually rubygems) has a cool feature where you can specify which version of a command you want to run;

If I run pod --version, it would execute with and output my default version (currently 1.8.0.beta.2), but when I run pod _1.7.5_ --version, it will give me 1.7.5.

Unfortunately the node/npm ecosystem doesn't provide such a mechanism.

But yes, I think it would be good for the library to define the required version of the cli, and let a script download and install that version.

scripts/run-bundled-codegen.sh Outdated Show resolved Hide resolved
APOLLO_CLI="${SCRIPT_DIR}/apollo/bin/run"

# Print version
echo "Apollo CLI Information: $(${APOLLO_CLI} --version)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the CLI isn't the fastest, requesting the version also adds a bit to the execution time. Might be something to consider.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is probably over-optimizing. It adds a trivial amount of time compared to how long codegen itself takes, and it'll make hunting down problems considerably clearer.

@@ -1,3 +1,5 @@
echo "warning: The check-and-run-apollo-cli.sh script is deprecated and will be removed. Please see https://www.apollographql.com/docs/ios/installation/#adding-a-code-generation-build-step for updated build instructions which use a bundled version of the Apollo CLI."
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think people will notice this message, since it will be tucked away in the build output.

You could exit with a non-zero code, to let the build fail, but then also touch a file, and then on the next invocation it won't fail anymore because it sees the touched file exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's why I prefaced this with warning, so it would show up in the Xcode sidebar and not just in the build log like so:

Screen Shot 2019-09-20 at 4 30 13 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, cool, didn't know about that 👍

@koenpunt
Copy link
Contributor

I think it might be good to keep it locked outside

One thing that came to mind; what if we make it a git-submodule, would that be supported by cocoapods, carthage and swift pm?

@designatednerd
Copy link
Contributor Author

what if we make it a git-submodule, would that be supported by cocoapods, carthage and swift pm?

I think maybe Carthage might support that, but I don't believe SPM does and I'm almost positive CP doesn't. Also the thought of having to deal with submodules ON TOP of three dependency managers is giving me a migraine just thinking about it.

I started thinking about having a repo as a dependency that just grabs the current binary and its contents, and then setting that up as a dependency here. My concern is that just pushes the repo bloat into a different repo that people are still going to have to check out, causing the exact same problem but with the additional overhead of having it be a separate repo.

@koenpunt
Copy link
Contributor

Well I'm just trying to suggest things to prevent us littering the main repo.

But the best solution IMO still is to upload the binary as a release asset to the github release, and download it from there.

@designatednerd
Copy link
Contributor Author

"Best" has a lot of possible meanings here, but the two biggest ones that are competing in this case are:

  • Is it the strategy that clogs up the repo the least?
  • Is it the strategy where developers can use the codegen the most easily?

The absolute easiest thing would be to just include the unzipped binaries directly in the repo, and then there are no extra steps in using it after downloading. However, you correctly pointed out that clogs up the repo from both a binary and file perspective, which is why we went to the .zip file with a script which extracts it. It's an extra step, but it's a local-only one which is not prone to many errors. People are still basically ready to build immediately after installing the dependency.

Having getting the CLI be a separate download step to get it means when people do pod install or carthage build or install via SPM, they are not ready to go immediately - they have to download the zip file and extract it. If they can't download it for whatever reason (no internet, GitHub is down, whatever artifact storage we're using is down), then they can't build.

If it only affected building Apollo stuff that'd be one thing, but because the version of the iOS SDK is tightly coupled to a particular version of the CLI, it will almost certainly kill the build for your whole project. Even if you have an earlier version of the CLI locally, the generated code and what the version of the SDK you have is expecting are going to be different, and you will almost certainly get errors at compile time. If you don't have an older version locally, then your code doesn't generate and you can't do anything. This is extra-true for CI builds, since there may be considerations about downloading to a CI machine that aren't necessarily the case for a local.

That scenario is definitely not as bad as the Node Hell™ we've been dealing with, but in terms of "Which of these is the lesser of two evils, particularly if you are dealing with developers new to the iOS ecosystem?", I'm willing to sacrifice some repo bloat to reduce the number of potential points of failure.

@koenpunt
Copy link
Contributor

I do understand what you want to achieve, but I think "Offline Support" is solving an annoying problem, but it's probably not a very common problem.

In terms of progressive enhancement we could start with downloading an archive from the script (as opposed to installing modules using npm, which also requires the network), which would already be a very big improvement to the workflow of the majority of users of the library.

And again, adding a binary to the repo with change its size indefinitely. Update the CLI 5 times, and users are pulling in 100mb of old CLI versions when they clone the repo, which is a lot of data for something they don't need. And you won't be able to remove this from the history, ever, without rewriting the git history, which is something you probably don't want. Old, but related post; https://robinwinslow.uk/2013/06/11/dont-ever-commit-binary-files-to-git/

@designatednerd
Copy link
Contributor Author

It's not just offline, though. Offline is the easiest-to-explain reason it'd happen, but "some way in which the developer cannot download the zip even though they were able to successfully download the rest of the dependency" is the problem I'm solving for here.

A few things I've seen problems with in the past that could cause issues even if the user has a perfectly good internet connection:

  • [Place hosting artifact download] goes down because of a bad deploy, and rolling back fails for several hours to days.
  • [Place hosting artifact download] gets DDOS'd and is inaccessible for several hours to days.
  • [Country developer lives in] decides to block off a huge block of IP addresses at the national level, which includes [place hosting artifact download].
  • [Place developer works] has an overly-agressive content filter which blocks [place hosting artifact download].

I'll mull this over with the team on Monday. The real way to have this not be in the repo is to use NPM, but we've seen that NPM is complex and unreliable enough, particularly for iOS developers, that we need a different solution.

@koenpunt
Copy link
Contributor

I think all four points come down to the same if we host the binaries on github as release assets, because then the same can happen for the initial clone.
And I'm not saying these things don't happen, but I think we're trying to solve a problem that shouldn't really be a concern of the library.

Also I don't think you responded to my size concern now or before (or did I maybe miss a comment somewhere in this thread?), but I'm interested to hear how you think that's going to work out over time.

@designatednerd
Copy link
Contributor Author

Somewhere in this nest of comments I basically said I'm fine with the repo bloat as a trade-off, but we're still discussing internally (I was out sick yesterday).

And while I get the thought about hosting the binaries as release assets on GH, that makes it really hard to do testing of new versions before we actually make a release, which is less than ideal.

@designatednerd
Copy link
Contributor Author

OK, here's the plan:

  • This PR will be merged as-is - it's a considerable improvement in developer experience that I really don't want to hold up any longer.
  • I will open an issue to look into future options in terms of potentially having the zip download from an artifact repository or a GH release resource in a way that can be used from any package manager.

TL;DR - We'll bloat the repo for now, but not for eternity.

@designatednerd
Copy link
Contributor Author

#791 has been created as a pin to come back to this.

@koenpunt
Copy link
Contributor

TL;DR - We'll bloat the repo for now, but not for eternity.

This isn't true, because the binaries we add, can never be removed, so will change the size of cloning the repo indefinitely. Or you must be fine with rewriting the history of the repo, but I can't imagine that's something you want?

But let's see what #791 yields.

@designatednerd
Copy link
Contributor Author

I meant the additions to the bloat will not continue eternally. I recognize what we're doing now will add some bloat.

@designatednerd designatednerd deleted the rm/node-nasties branch September 25, 2019 22:37
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.

3 participants