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

chore(deps): specify versions for @types/express and friends #4493

Merged
merged 2 commits into from
Aug 28, 2020

Conversation

glasser
Copy link
Member

@glasser glasser commented Aug 19, 2020

In 91b8839 @abernix updated package-lock for @types/express (something I had
also noted happened on a branch I was working on). Soon after, in #4327, that
line got dropped from package-lock.json.

I'm guessing this caused the issue I started seeing soon afterwards, where I'd
get lots of compile errors relating to @types/express and other
DefinitelyTyped packages it depends on. Turns out the dependencies from
@types/express are unversioned, but the latest versions of that type package
depends on the latest versions of a bunch of other type packages.

This commit seems to fix things for me.

In 91b8839 @abernix updated package-lock for `@types/express` (something I had
also noted happened on a branch I was working on). Soon after, in #4327, that
line got dropped from package-lock.json.

I'm guessing this caused the issue I started seeing soon afterwards, where I'd
get lots of compile errors relating to `@types/express` and other
DefinitelyTyped packages it depends on. Turns out the dependencies from
`@types/express` are unversioned, but the latest versions of that type package
depends on the latest versions of a bunch of other type packages.

This commit seems to fix things for me.
@abernix abernix self-requested a review August 20, 2020 17:05
- We don't directly depend on `@types/express-serve-static-core`, so I don't
  quite understand the addition of those.  I seem to not have the flapping
  of deps without adding it!

- We shouldn't need to package the `@types/qs` (or the aforementioned pkg)
  into the `apollo-server-express` package's non-dev deps because none of
  those types are exported from our emitted `d.ts` files.  Those
  dependencies should be brought by the corresponding packages if there are
  transitive dep needs.
@glasser
Copy link
Member Author

glasser commented Aug 28, 2020

Hmm, it does suck that the dependencies from @types/express on things like @types/express-serve-static-core
have no version constraint, so it does seem like working around that by specifying those deps in our package may be required. On the other hand one would hope that package-lock is good enough to at least make things more reproducible than I was seeing. I don't understand this stuff well enough to evaluate your changes but they seem reasonable enough. If they don't fix the problem we can always try pinning transitive dep versions later.

Thanks for explaining that the reason for @types non-dev dependencies is if the generated .d.ts packages depend on them.

@abernix abernix merged commit 3f2f414 into main Aug 28, 2020
@glasser
Copy link
Member Author

glasser commented Sep 14, 2020

FWIW when building an app depending on tarballs from #4453, I did run into errors due to the version of @types/express-serve-static-core, which I only was able to fix by pinning that version in my app's devDependencies. While abstractly it doesn't seem necessary, I do think providing the version dependency may help our users deal with the fact that DefinitelyTyped makes backwards-incompatible changes without actually specifying version ranges.

glasser added a commit that referenced this pull request Sep 21, 2020
This is another attempt to solve the problem of #4493.

The problem is that the new version of `@types/express` we use in this release
actually does require a newer version of `@types/express-serve-static-core`, but
it only uses `*` as a version constraint. So while from-scratch installs of
`apollo-server-express` will pull in the newest version of
`@types/express-serve-static-core`, upgrades will only bother to upgrade
`@types/express` and TypeScript will fail.
@abernix abernix deleted the glasser/fix-types-express branch December 31, 2020 10:58
abernix added a commit that referenced this pull request Mar 7, 2021
...with a little extra help from @abernix because these types seem like
they're acting up again.  This time, I've found another issue on
DefinitelyTyped that seems like it's prescribing a solution that was
occurring in this particular attempt at updating the types, resulting in the
error message of:

  Namespace 'serveStatic' has no exported member 'RequestHandlerConstructor'

The (attempted, but seemingly working) fix was:

    npm update @types/express-serve-static-core --depth 2
    npm update @types/serve-static --depth 2

Ref: DefinitelyTyped/DefinitelyTyped#49595

But also, in reverse chronological order at attempted resolution:

Ref: 6e86f67
Ref: #4493
Ref: c67e8ec
Cc: @glasser
abernix added a commit that referenced this pull request Mar 7, 2021
...with a little extra help from @abernix because these types seem like
they're acting up again.  This time, I've found another issue on
DefinitelyTyped that seems like it's prescribing a solution that was
occurring in this particular attempt at updating the types, resulting in the
error message of:

  Namespace 'serveStatic' has no exported member 'RequestHandlerConstructor'

The (attempted, but seemingly working) fix was:

    npm update @types/express-serve-static-core --depth 2
    npm update @types/serve-static --depth 2

Ref: DefinitelyTyped/DefinitelyTyped#49595

But also, in reverse chronological order at attempted resolution:

Ref: 6e86f67
Ref: #4493
Ref: c67e8ec
Cc: @glasser

Co-authored-by: Jesse Rosenberger <git@jro.cc>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants