Builds fail in pnpm projects using @loopback/rest 1.0.1

[mgabeler-lee-6rs]

Description / Steps to reproduce / Feature proposal

Trying to use loopback 4 1.0.0/1 in a pnpm project.

Current Behavior

Project fails to build due to @loopback/rest package relying on npm providing transitive dependencies to it, specifically for serve-static and express-serve-static-core

Expected Behavior

@loopback/ packages should declare all their dependencies so that they work with stricter package managers like pnpm

Example error output:

> lb-tsc es2018

../../common/temp/node_modules/.registry.npmjs.org/@loopback/rest/1.0.1/node_modules/@loopback/rest/dist/src/rest.application.d.ts:8:36 - error TS2307: Cannot find module 'serve-static'.

8 import { ServeStaticOptions } from 'serve-static';
                                     ~~~~~~~~~~~~~~

../../common/temp/node_modules/.registry.npmjs.org/@loopback/rest/1.0.1/node_modules/@loopback/rest/dist/src/rest.application.d.ts:9:28 - error TS2307: Cannot find module 'express-serve-static-core'.

9 import { PathParams } from 'express-serve-static-core';
                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~

../../common/temp/node_modules/.registry.npmjs.org/@loopback/rest/1.0.1/node_modules/@loopback/rest/dist/src/rest.server.d.ts:8:28 - error TS2307: Cannot find module 'express-serve-static-core'.

8 import { PathParams } from 'express-serve-static-core';
                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~

../../common/temp/node_modules/.registry.npmjs.org/@loopback/rest/1.0.1/node_modules/@loopback/rest/dist/src/rest.server.d.ts:10:36 - error TS2307: Cannot find module 'serve-static'.

10 import { ServeStaticOptions } from 'serve-static';
                                      ~~~~~~~~~~~~~~

The ../../common/temp/... is because this is also a project in a rush monorepo

Workaround

Add packages to my project's dependencies:

• serve-static
• express-serve-static-core
  • Actually it turns out it's very important I do not add this as a dependency of my project, because this package only exports types ... wrong types!
• @types/serve-static
• @types/express-serve-static-core
  • This one is perversely important, as otherwise the types built into express-serve-static-core (which is the sole content of that module) get used, which are wrong

[raymondfeng]

@mgabeler-lee-6rs Thank you for the reporting the issue. Would you like to submit a patch?

[mgabeler-lee-6rs]

Working on it ... trying to work out the minimal / proper fix and a good test case to at least verify my patch locally

[mgabeler-lee-6rs]

What I'm not sure how to handle is that I think @loopback/rest wants to generally import the same version of serve-static that is used by whichever version of express it's using. It doesn't seem easy to express that other than by manual maintenance of the package.json file.

It seems that serve-static is a pretty rarely updated package, so perhaps just caret-versioning it like is done with express is enough, and hopefully should have the desired results.

[bajtos]

Thank you @mgabeler-lee-6rs for reporting the problem and sending a pull request to fix it.

To be honest, I am confused on why we have to include serve-static as our dependency when express has already declared it in its dependencies (see here) and we are always accessing serve-static via the API provided by express (express.static()).

IMO, it should be enough to list the two @types packages as the dependency of @loopback/rest. (It would be even better if @types/express were including those two types packages OOTB so that we don't have to explicitly add them to our dev-deps.)

Thoughts?

[mgabeler-lee-6rs]

While you're only importing types from serve-static, I suspect that, in an environment I have where transitive dependencies are not implicitly available, the import from serve-static would still fail. I suppose it might be possible to import the type from @types/serve-static, but that would be ... weird?

I'll look into this some to see if I can find a slimmer solution than the one merged here.

[bajtos]

While you're only importing types from serve-static, I suspect that, in an environment I have where transitive dependencies are not implicitly available, the import from serve-static would still fail. I suppose it might be possible to import the type from @types/serve-static, but that would be weird?

I think we need to distinguish between compile-time and run-time.

At compile-time, we are importing type definitions for configuration options directly from serve-static because Express typings don't re-export them :( That's why we need to explicitly depend on @types/serve-static and @types/express-serve-static-core, this is the part I am fine with.

Since we are loading the typings only, I don't see what would trigger a load of serve-static javascript source code (that's not be present in your environment if it is not explicitly listed as LB dependency).

https://github.com/strongloop/loopback-next/search?q=serve-static&unscoped_q=serve-static

At runtime, we call express.static() which loads serve-static from the context of express module which does directly depend on serve-static, thus I believe everything should work as intended.

I'll look into this some to see if I can find a slimmer solution than the one merged here.

That would be great 👍

I think the only change we need here is to remove serve-static from our dependencies. Could you please give it a try? I don't have any pnpm project where I would be able to test this out myself.

[mgabeler-lee-6rs]

Since we are loading the typings only, I don't see what would trigger a load of serve-static javascript source code

The lines like this, from your code search, will try to load the actual code, unless I'm seriously missing something:

import {ServeStaticOptions} from 'serve-static';

Even though only the types are needed, it's still going to try to load up the module code, including at runtime, from what I understand. It certainly tries to do so at build time -- after all, tsc has no way to know that ServeStaticOptions is just an interface and not an actual class with code that will be needed, right?

In the context of rush + pnpm, you cannot require/import transitive dependencies -- it does a somewhat funny setup of your node_modules directories in part to explicitly prevent relying on transitive dependencies. I think even bare pnpm does some of this, by design. Consider the general case, where non-exported material from a packages dependencies is not usually considered part of its API contract, so it could change those out completely at will even in a minor patch release.

[raymondfeng]

import {ServeStaticOptions} from 'serve-static'; just loads the TS definition if ServeStaticOptions is a TS type instead of value.

[mgabeler-lee-6rs]

Huh ... I guess so! I'll submit a second PR to clean that up!

[mgabeler-lee-6rs]

Well, it seems that this didn't actually fix the problem :(

I think the issue is that I added them to the devDependencies ... which aren't installed when pulling in @loopback as a normal dependency.