firebase doesn't play nicely with `yarn install --flat`

[zevdg]

[REQUIRED] Describe your environment

• Operating System version: Linux 4.15.7 (Solus 3)
• Firebase SDK version: npm firebase@4.10.1
• Firebase Product: firestore

[REQUIRED] Describe the problem

I was trying to follow best practices with my web app and ran yarn install --prod --flat to make sure that I wasn't using delivering multiple copies of the same libraries to my clients. I found several libraries with conflicts and they were all being pulled in by firebase. Specifically: minimist, readable-stream, isarray, process-nextick-args, string_decoder, assert-plus, extsprintf, glob, through2, vinyl, clone, clone-stats, replace-ext, glob-parent, is-glob, is-extglob, kind-of, and is-number.

Some further debugging...

$ npm ls --prod minimist readable-stream isarray process-nextick-args string_decoder assert-plus extsprintf glob through2 vinyl clone clone-stats replace-ext glob-parent is-glob is-extglob kind-of is-number
site@1.0.0 /home/user/workspace/site
└─┬ firebase@4.10.1
  ├─┬ @firebase/firestore@0.3.4
  │ └─┬ grpc@1.9.1
  │   ├─┬ node-pre-gyp@0.6.39
  │   │ ├─┬ mkdirp@0.5.1
  │   │ │ └── minimist@0.0.8
  │   │ ├─┬ npmlog@4.1.2
  │   │ │ └─┬ are-we-there-yet@1.1.4
  │   │ │   └── readable-stream@2.3.3  deduped
  │   │ ├─┬ rc@1.2.4
  │   │ │ └── minimist@1.2.0
  │   │ ├─┬ request@2.81.0
  │   │ │ └─┬ http-signature@1.1.1
  │   │ │   ├── assert-plus@0.2.0
  │   │ │   ├─┬ jsprim@1.4.1
  │   │ │   │ ├── assert-plus@1.0.0
  │   │ │   │ ├── extsprintf@1.3.0
  │   │ │   │ └─┬ verror@1.10.0
  │   │ │   │   ├── assert-plus@1.0.0
  │   │ │   │   └── extsprintf@1.3.0  deduped
  │   │ │   └─┬ sshpk@1.13.1
  │   │ │     ├── assert-plus@1.0.0
  │   │ │     ├─┬ dashdash@1.14.1
  │   │ │     │ └── assert-plus@1.0.0
  │   │ │     └─┬ getpass@0.1.7
  │   │ │       └── assert-plus@1.0.0
  │   │ ├─┬ rimraf@2.6.2
  │   │ │ └── glob@7.1.2
  │   │ └─┬ tar-pack@3.4.1
  │   │   └─┬ readable-stream@2.3.3
  │   │     ├── isarray@1.0.0
  │   │     ├── process-nextick-args@1.0.7
  │   │     └── string_decoder@1.0.3
  │   └─┬ protobufjs@5.0.2
  │     └── glob@7.1.2
  └─┬ @firebase/messaging@0.2.1
    └─┬ lcov-result-merger@2.0.0
      ├─┬ through2@2.0.3
      │ └─┬ readable-stream@2.3.5
      │   ├── isarray@1.0.0
      │   ├── process-nextick-args@2.0.0
      │   └── string_decoder@1.0.3
      ├─┬ vinyl@2.1.0
      │ ├── clone@2.1.1
      │ ├── clone-stats@1.0.0
      │ ├─┬ cloneable-readable@1.0.0
      │ │ ├── process-nextick-args@1.0.7
      │ │ └── through2@2.0.3  deduped
      │ └── replace-ext@1.0.0
      └─┬ vinyl-fs@2.4.4
        ├─┬ duplexify@3.5.4
        │ └── readable-stream@2.3.5  deduped
        ├─┬ glob-stream@5.3.5
        │ ├── glob@5.0.15
        │ ├─┬ glob-parent@3.1.0
        │ │ └─┬ is-glob@3.1.0
        │ │   └── is-extglob@2.1.1
        │ ├─┬ micromatch@2.3.11
        │ │ ├─┬ braces@1.8.5
        │ │ │ └─┬ expand-range@1.8.2
        │ │ │   └─┬ fill-range@2.2.3
        │ │ │     ├─┬ is-number@2.1.0
        │ │ │     │ └── kind-of@3.2.2  deduped
        │ │ │     ├─┬ isobject@2.1.0
        │ │ │     │ └── isarray@1.0.0  deduped
        │ │ │     └─┬ randomatic@1.1.7
        │ │ │       ├─┬ is-number@3.0.0
        │ │ │       │ └── kind-of@3.2.2
        │ │ │       └── kind-of@4.0.0
        │ │ ├─┬ extglob@0.3.2
        │ │ │ └── is-extglob@1.0.0
        │ │ ├── is-extglob@1.0.0
        │ │ ├─┬ is-glob@2.0.1
        │ │ │ └── is-extglob@1.0.0  deduped
        │ │ ├── kind-of@3.2.2
        │ │ └─┬ parse-glob@3.0.4
        │ │   ├─┬ glob-base@0.3.0
        │ │   │ ├─┬ glob-parent@2.0.0
        │ │   │ │ └── is-glob@2.0.1  deduped
        │ │   │ └─┬ is-glob@2.0.1
        │ │   │   └── is-extglob@1.0.0
        │ │   ├── is-extglob@1.0.0
        │ │   └─┬ is-glob@2.0.1
        │ │     └── is-extglob@1.0.0  deduped
        │ ├─┬ ordered-read-streams@0.3.0
        │ │ └── readable-stream@2.3.5  deduped
        │ └─┬ through2@0.6.5
        │   └─┬ readable-stream@1.0.34
        │     ├── isarray@0.0.1
        │     └── string_decoder@0.10.31
        ├─┬ gulp-sourcemaps@1.6.0
        │ ├── through2@2.0.3  deduped
        │ └─┬ vinyl@1.2.0
        │   ├── clone@1.0.3
        │   ├── clone-stats@0.0.1
        │   └── replace-ext@0.0.1
        ├─┬ lazystream@1.0.0
        │ └── readable-stream@2.3.5  deduped
        ├─┬ merge-stream@1.0.1
        │ └── readable-stream@2.3.5  deduped
        ├─┬ mkdirp@0.5.1
        │ └── minimist@0.0.8
        ├── readable-stream@2.3.5  deduped
        ├── through2@2.0.3  deduped
        ├─┬ through2-filter@2.0.0
        │ └── through2@2.0.3  deduped
        └─┬ vinyl@1.2.0
          ├── clone@1.0.3
          ├── clone-stats@0.0.1
          └── replace-ext@0.0.1

As you can see, all of these conflicts are getting pulled in due to grpc getting pulled in by @firebase/firestore and lcov-result-merger getting pulled in by @firebase/messaging.

The grpc package ultimately gets tree-shaken out on non-node platforms, so it really isn't a problem. However, since it's listed as a normal dependency, yarn can't know that it will ultimately get excluded from the build and so it gets included in the --flat resolution. I'm not sure what the best solution to this problem is, but I suspect the easiest thing would be to make it an optional dependency.

The lcov-result-merger package appears to have been added as part of this commit by mistake. It's only used as a cli tool during development and it's already included as a dev dependency at the root of the project. I removed it from @firebase/messaging and all the tests still pass.

Steps to reproduce:

#vue isn't necessary to reproduce this.  This was just one of the simplest demos I could find that actually imported firebase as an npm package 
git clone git@github.com:Q42/vuejs-firestore-demo.git 
cd vuejs-firestore-demo
npm upgrade firebase

# then, to see the issue before we get yarn involved
npm ls --prod minimist readable-stream isarray process-nextick-args string_decoder assert-plus extsprintf glob through2 vinyl clone clone-stats replace-ext glob-parent is-glob is-extglob kind-of is-number

# to recreate the issue itself, run
yarn install --prod --flat

Relevant Code:

see reproduction steps

[mikelehen]

Making grpc an optionalDependency doesn't seem like a good solution to me, since it may result in the client being broken on node. We might be able to exclude it from the browser with something like this in package.json:

"browser": {
  ...
  "grpc": false
}

But I don't know if that helps the yarn flat issue or not...

Else, perhaps the correct solution is to work with grpc and other dependencies to update their dependencies to compatible versions... though I don't know how big of an undertaking that is.

[zevdg]

Unfortunatly, browser isn't a standard field. Neither npm or yarn support it https://github.com/stereobooster/package.json#browser

Getting grpc to clean up it's dependencies would be nice, but expecting server-side modules like grpc to do work that really only benefits client-side projects is a hard sell. They would most likely respond with, why are you pulling grpc into a client-side dependency tree in the first place? Stop that! And I would agree with them. Server-side and client-side javascript are 2 totally different beasts and the real issue here is firestore trying to force them both into the same project.

Even if grpc agreed to do this work, there already are pull requests for grpc dependencies like https://github.com/substack/node-mkdirp/pull/124 that have been ignored for years.

IMO, large conditional dependencies like this are a code smell. Dependency management is hard enough for tooling to handle without libraries pulling in tons of code that a downstream developer knows that it will never use. In cases like this where the condition is always known in advance, simply having 2 separate packages that each only depend on what they actually need (in this case, firestore-web and firestore-node or something like that) is the simplest solution and the best one IMO. When that's not possible, the next best option is to require the calling code to inject the conditional dependency during initialization if they need it.

However, both of those would be an API change so I assume that ship has already sailed. 😞
Technically firestore is still beta, so maybe not? 🤞

Assuming the conditional grpc dep cannot be refactored out, and without a well supported "browser" build tag, optionalDependency appears to be the only other option. AFAICT, optional dependencies are still installed by default by both yarn and npm, so it this would only affect people who are using firestore on the server side AND are already using the --no-optional flag in their build for other reasons. They would just need to add grpc as a direct dependency of their project to resolve the issue. I wouldn't expect enough people to fall into this category for it to be a serious problem. The only other thing that optionalDependency does is to prevent the build from failing if grpc couldn't be installed for a more legitimate reason (like a network error). This isn't great, but it's still the lesser of 2 evils.

[mikelehen]

The only other thing that optionalDependency does is to prevent the build from failing if grpc couldn't be installed for a more legitimate reason (like a network error)

That was my main concern. grpc can fail to install for a variety of reasons since it's based on a C core that must be compiled if there isn't a prebuilt binary for your platform. We've seen it fail before and if yarn / npm silently ignored this, it could lead to broken users and support headaches.

I'll let @jshcrowthe weigh in with his thoughts. My objection is largely in principle (grpc is not optional and node code will break if it's not installed). If we think the actual user experience is acceptable, then this change might be okay.

[zevdg]

Eww, I see how that would be a problem. Frankly, I'm not particularly happy with the optional deps solution for other reasons I didn't go into.

I do hope the package splitting or dependency injection solutions are viable alternatives since yarn doesn't seem open to adding support for the browser field.

[jshcrowthe]

Hey @zevdg thanks a bunch for the PR here and trying to help us figure this out!

I agree with @mikelehen that moving grpc to an optionalDependency is probably not the right solution. It's important to note that grpc is not an optional dependency, it is a runtime dependency for Node and w/o it this package won't function in Node environments.

While trying to install the dependencies using the --flat flag will allow you to only include a single version of a given dependency, you assume the risk that any given resolution could break your dependencies. Just doing yarn install --prod will still give you much of the behavior you need (i.e. install deps, resolve all compatible version ranges to the same instance of a dependency), w/o the risk of breaking your dependencies by changing the versions out from under the authors intention.

For Node.js, multiple instances of a dep isn't a huge concern, as the dependency size isn't nearly as large of a factor (though, admittedly, it still has an impact). I think this is really more of a "the JS community ships isomorphic dependencies (with different assumptions about dependencies) to the Node Package Manager" problem, than a Firebase problem. An alternate solution could be to work w/ the yarn team to allow you to ignore specific dependencies (and their transitive deps) in --flat resolution. Or just ignore those dependencies entirely, if you know that the package is shipping some things that aren't needed for your target environment.

All that said, I think your best bet is to probably to avoid passing the --flat flag until yarn supports ignoring env specific deps, or until the community standardizes around a better solution for delineating between isomorphic/browser/node dependencies.

[zevdg]

Thanks! I'd agree that yarn could certainly provide some additional fine tuning knobs around --flat.

As a side note, the polymer 3 preview currently requires the use of this --flat flag. So until this is resolved one way or another, firestore and polymer3 cannot be used together ☹️. The discussion about how tooling should handle polymer3 and flattening is ongoing over at package-community/discussions#2 so you might want to keep an eye on that. It'd be a shame if 2 of my favorite Google projects couldn't be used together.