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

[@types/serve-static] Remove dependency on @types/mime #69231

Merged
merged 1 commit into from
Apr 2, 2024

Conversation

ghost
Copy link

@ghost ghost commented Apr 1, 2024

Please fill in this template.

If changing an existing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: <>
  • If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the package.json.

@typescript-bot typescript-bot added this to Needs Author Action in New Pull Request Status Board Apr 1, 2024
@ghost ghost force-pushed the patch-16 branch 2 times, most recently from 86622b8 to 2cc6b66 Compare April 1, 2024 07:39
@ghost ghost marked this pull request as ready for review April 1, 2024 07:39
@typescript-bot
Copy link
Contributor

typescript-bot commented Apr 1, 2024

@codershiba Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

Code Reviews

Because this is a widely-used package, a DT maintainer will need to review it before it can be merged.

You can test the changes of this PR in the Playground.

Status

  • ✅ No merge conflicts
  • ✅ Continuous integration tests have passed
  • ✅ Only a DT maintainer can approve changes without tests

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.


Diagnostic Information: What the bot saw about this PR
{
  "type": "info",
  "now": "-",
  "pr_number": 69231,
  "author": "codershiba",
  "headCommitOid": "9df89f8a703e49ffc7f88d56d04140aff52372fd",
  "mergeBaseOid": "41aa59f8e323a93b98ecea69fcf67a7a0a2cfa67",
  "lastPushDate": "2024-04-01T06:12:31.000Z",
  "lastActivityDate": "2024-04-02T16:29:49.000Z",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "serve-static",
      "kind": "edit",
      "files": [
        {
          "path": "types/serve-static/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/serve-static/package.json",
          "kind": "package-meta-ok"
        }
      ],
      "owners": [
        "urossmolnik",
        "LinusU",
        "devanshj"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "jakebailey",
      "date": "2024-04-02T16:29:49.000Z",
      "isMaintainer": true
    }
  ],
  "mainBotCommentID": 2029322084,
  "ciResult": "pass"
}

@typescript-bot
Copy link
Contributor

🔔 @urossmolnik @LinusU @devanshj — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

@typescript-bot typescript-bot moved this from Needs Author Action to Waiting for Code Reviews in New Pull Request Status Board Apr 1, 2024
@typescript-bot
Copy link
Contributor

typescript-bot commented Apr 1, 2024

@codershiba Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

Code Reviews

Because this is a widely-used package, a DT maintainer will need to review it before it can be merged.

You can test the changes of this PR in the Playground.

Status

  • ✅ No merge conflicts
  • ✅ Continuous integration tests have passed
  • ✅ Only a DT maintainer can approve changes without tests

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.


Diagnostic Information: What the bot saw about this PR
{
  "type": "info",
  "now": "-",
  "pr_number": 69231,
  "author": "codershiba",
  "headCommitOid": "9df89f8a703e49ffc7f88d56d04140aff52372fd",
  "mergeBaseOid": "41aa59f8e323a93b98ecea69fcf67a7a0a2cfa67",
  "lastPushDate": "2024-04-01T06:12:31.000Z",
  "lastActivityDate": "2024-04-02T16:29:49.000Z",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "serve-static",
      "kind": "edit",
      "files": [
        {
          "path": "types/serve-static/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/serve-static/package.json",
          "kind": "package-meta-ok"
        }
      ],
      "owners": [
        "urossmolnik",
        "LinusU",
        "devanshj"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "jakebailey",
      "date": "2024-04-02T16:29:49.000Z",
      "isMaintainer": true
    }
  ],
  "mainBotCommentID": 2029322084,
  "ciResult": "pass"
}

@typescript-bot
Copy link
Contributor

🔔 @urossmolnik @LinusU @devanshj — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Maintainer Review in New Pull Request Status Board Apr 1, 2024
@jakebailey
Copy link
Member

Why?

@esetnik
Copy link
Contributor

esetnik commented Apr 1, 2024

I think the correct fix is in a219ae4 since mime v4 ships with it's own types that may end up being incompatible with @types/serve-static.

@ghost ghost marked this pull request as draft April 2, 2024 01:03
@typescript-bot typescript-bot moved this from Needs Maintainer Review to Needs Author Action in New Pull Request Status Board Apr 2, 2024
@stu43005
Copy link

stu43005 commented Apr 2, 2024

I think the most accurate solution would be to change the dependency to @types/send.

Because serve-static actually depends on send package instead of mime.

see: https://github.com/expressjs/serve-static/blob/9b5a12a76f4d70530d2d2a8c7742e9158ed3c0a4/index.js#L29

Edit:
And @types/send has the correct dependency on @types/mime@^1 !

@DangerBotOSS
Copy link

DangerBotOSS commented Apr 2, 2024

Formatting

The following files are not formatted:

  1. types/serve-static/index.d.ts

Consider running pnpm dprint fmt on these files to make review easier.

Generated by 🚫 dangerJS against 9df89f8

@ghost ghost mentioned this pull request Apr 2, 2024
7 tasks
@ghost ghost marked this pull request as ready for review April 2, 2024 11:00
@typescript-bot typescript-bot added the Untested Change This PR does not touch tests label Apr 2, 2024
@typescript-bot typescript-bot moved this from Needs Author Action to Waiting for Code Reviews in New Pull Request Status Board Apr 2, 2024
@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Maintainer Review in New Pull Request Status Board Apr 2, 2024
@AviVahl
Copy link
Contributor

AviVahl commented Apr 2, 2024

Why?

When #69213 landed, half the web broke. That's why. :)
@types/express use @types/serve-static. @types/serve-static used "@types/mime": "*", which broke once @types/mime@4 released.

Like @stu43005 wrote, serve-static actually uses send, which uses an older mime package version (and its relevant types).

refs:
firebase/firebase-admin-node#2512
googleapis/nodejs-storage#2434
fastify/send#60

@jakebailey
Copy link
Member

jakebailey commented Apr 2, 2024

Then we should revert #69213 then simply delete it without a shim. @types/mime was far too large to publish a shim for.

@jakebailey
Copy link
Member

No, nevermind, the shim package is fine as it only applied to mime v4. But we should fix this package.

@jakebailey
Copy link
Member

I'm not sure why moving it to send helps, though; that package is still using @types/mime@*.

@AviVahl
Copy link
Contributor

AviVahl commented Apr 2, 2024

TBH, it looks like the change here is the correct way to go.
mime@4 does have built-in types, so it should probably follow the usual procedure of deprecating a @types packages.

It's just that packages that depended on it using "*" need to get fixed.

@AviVahl
Copy link
Contributor

AviVahl commented Apr 2, 2024

I'm not sure why moving it to send helps, though; that package is still using @types/mime@*.

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/send/package.json
"@types/mime": "^1",

@jakebailey
Copy link
Member

I apparently can't read!

@typescript-bot typescript-bot added Maintainer Approved Self Merge This PR can now be self-merged by the PR author or an owner labels Apr 2, 2024
@typescript-bot typescript-bot moved this from Needs Maintainer Review to Waiting for Author to Merge in New Pull Request Status Board Apr 2, 2024
@jakebailey jakebailey merged commit 0cac737 into DefinitelyTyped:master Apr 2, 2024
8 checks passed
@typescript-bot typescript-bot removed this from Waiting for Author to Merge in New Pull Request Status Board Apr 2, 2024
@AviVahl
Copy link
Contributor

AviVahl commented Apr 2, 2024

Thank you for merging.
master is green, but no new @types/serve-static release yet...?
https://www.npmjs.com/package/@types/serve-static

@ghost
Copy link
Author

ghost commented Apr 2, 2024

Thank you for merging. master is green, but no new @types/serve-static release yet...? https://www.npmjs.com/package/@types/serve-static

Takes some time I guess

@jakebailey
Copy link
Member

Publishing happens every half hour; give it like 5 minutes or so.

@ghost ghost deleted the patch-16 branch April 2, 2024 17:00
@ghost
Copy link
Author

ghost commented Apr 2, 2024

Sorry for breaking this and causing so much chaos but I guess this was the only major package here that depending on @types/mime: "*"

@jakebailey
Copy link
Member

All good, our tooling unfortunately can't detect this kind of break because at the point at which a package is removed, the dummy package has not yet been published to npm. A known downside of the new pnpm layout that we need to figure out, probably by erroring on * deps which are present in notNeededPackages.

@stu43005
Copy link

stu43005 commented Apr 3, 2024

Just a note

This PR changes the import to node:http, which only supports @types/node@>=14.18.0.

@jakebailey
Copy link
Member

Feel free to send a PR and tag me, that seemed like an unnecessary change I missed.

@jakebailey
Copy link
Member

Actually, just sent #69258

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Critical package Maintainer Approved Self Merge This PR can now be self-merged by the PR author or an owner Untested Change This PR does not touch tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants