fix(resolve): add `readPackage(Sync)` #51182

SimenB · 2021-02-11T15:07:12Z

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: [New] add readPackage and readPackageSync browserify/resolve#236
If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.

typescript-bot · 2021-02-11T15:07:49Z

@SimenB Thank you for submitting this PR!

This is a live comment which I will keep updated.

2 packages in this PR

browser-resolve on npm, on unpkg
resolve on npm, on unpkg
- 1 added owner: @ljharb

Code Reviews

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

Status

✅ No merge conflicts
✅ Continuous integration tests have passed
✅ A DT maintainer needs to approve changes which affect more than one package

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": 51182,
  "author": "SimenB",
  "headCommitOid": "faf0de9d9a3b626d478c1c29329a6323f22ba405",
  "lastPushDate": "2021-02-13T08:23:53.000Z",
  "lastActivityDate": "2021-02-18T16:49:46.000Z",
  "maintainerBlessed": false,
  "mergeOfferDate": "2021-02-18T16:40:55.000Z",
  "mergeRequestDate": "2021-02-18T16:49:46.000Z",
  "mergeRequestUser": "SimenB",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "browser-resolve",
      "kind": "edit",
      "files": [
        {
          "path": "types/browser-resolve/browser-resolve-tests.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "marionebl",
        "peterblazejewicz"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Popular"
    },
    {
      "name": "resolve",
      "kind": "edit",
      "files": [
        {
          "path": "types/resolve/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/resolve/resolve-tests.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "marionebl",
        "ajafff"
      ],
      "addedOwners": [
        "ljharb"
      ],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "orta",
      "date": "2021-02-18T16:40:19.000Z",
      "isMaintainer": true
    },
    {
      "type": "approved",
      "reviewer": "ljharb",
      "date": "2021-02-13T14:39:45.000Z",
      "isMaintainer": false
    },
    {
      "type": "stale",
      "reviewer": "rbuckton",
      "date": "2021-02-13T03:06:35.000Z",
      "abbrOid": "6b7be4a"
    }
  ],
  "ciResult": "pass"
}

typescript-bot · 2021-02-11T15:07:50Z

🔔 @marionebl @peterblazejewicz @ajafff — 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.

SimenB · 2021-02-11T15:07:55Z

types/resolve/index.d.ts

  }

  export interface SyncOpts extends Opts {
    /** how to read files synchronously (defaults to fs.readFileSync) */
-    readFileSync?: (file: string, encoding: BufferEncoding) => string | Buffer;


encoding is not passed: https://github.com/browserify/resolve/blob/2e2d2630122a8e6a3205f9a8baa5be50ab7b3108/lib/sync.js#L49

typescript-bot · 2021-02-11T15:12:10Z

@SimenB The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

typescript-bot · 2021-02-11T15:22:14Z

👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings.

Let’s review the numbers, shall we?

These typings are for a version of resolve that doesn’t yet exist on master, so I’ve compared them with v1.19.

Comparison details 📊

	1.19@master	1.20 in #51182	diff
Batch compilation
Memory usage (MiB)	66.5	67.0	+0.7%
Type count	8519	8527	0%
Assignability cache size	920	909	-1%

Language service
Samples taken	161	161	0%
Identifiers in tests	161	161	0%
`getCompletionsAtPosition`
Mean duration (ms)	363.7	351.2	-3.5%
Mean CV	10.2%	11.2%
Worst duration (ms)	424.3	442.3	+4.2%
Worst identifier	stat	stat
`getQuickInfoAtPosition`
Mean duration (ms)	365.9	352.0	-3.8%
Mean CV	9.6%	10.5%
Worst duration (ms)	460.4	410.0	-11.0%
Worst identifier	resolve	isFile

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

typescript-bot · 2021-02-11T15:27:29Z

@SimenB The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

SimenB · 2021-02-11T15:29:08Z

types/resolve/resolve-tests.ts

+    });
+    resolved = resolve.sync('typescript', {
+        readPackageSync(readFileSync, file) {
+            return JSON.parse(readFileSync(file));


JSON.parse in node can take a Buffer, so the type error is wrong. Should I just cast it?

$ node -p 'JSON.parse(Buffer.from(JSON.stringify({ works: true})))' { works: true }

/cc @orta

JSON.parse only takes in a Buffer because it does .toString() on the input (https://tc39.es/ecma262/#sec-json.parse, step 1). JSON.parse(1) and JSON.parse(null) also "work", but that doesn't mean we should widen the input to accept anything. I'd recommend being more explicit (i.e., readFileSync(file, "utf8") or JSON.parse(readFileSync(file).toString("utf8")), etc.

Either way, the cast is fine in this case since we're testing the types, not the runtime behavior.

If it takes any toStringable, then the proper type is string | { toString: () => string }, no?

If it takes any toStringable, then the proper type is string | { toString: () => string }, no?

I went for that, with the nice side effect of removing the dependency on @types/node

3e4ee0a (#51182)

ljharb

LGTM, but it’d be nice to capture the mutual exclusivity of readPackage/readFile

types/resolve/index.d.ts

Co-authored-by: Jordan Harband <ljharb@gmail.com>

SimenB · 2021-02-11T15:41:03Z

it’d be nice to capture the mutual exclusivity of readPackage/readFile

can probably use a union type for the options. will fix at the same time when addressing #51182 (comment)

typescript-bot · 2021-02-11T15:41:49Z

@SimenB The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

typescript-bot · 2021-02-11T15:56:35Z

@SimenB The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

typescript-bot · 2021-02-13T08:28:53Z

@ljharb, @rbuckton Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

typescript-bot · 2021-02-13T14:40:20Z

@rbuckton Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

SimenB · 2021-02-18T15:55:40Z

Ping @marionebl @peterblazejewicz @ajafff @rbuckton 🙂

typescript-bot · 2021-02-18T16:40:55Z

@SimenB Everything looks good here. Great job! I am ready to merge this PR (at faf0de9) on your behalf.

If you'd like that to happen, please post a comment saying:

Ready to merge

and I'll merge this PR almost instantly. Thanks for helping out! ❤️

SimenB · 2021-02-18T16:49:46Z

Ready to merge

typescript-bot · 2021-02-18T17:25:54Z

I just published @types/resolve@1.20.0 to npm.

@SimenB

…` by @SimenB * fix(resolve): add readPackage(Sync) * Update resolve-tests.ts * Update types/resolve/index.d.ts Co-authored-by: Jordan Harband <ljharb@gmail.com> * maybe fix ci * use never to force mutual exclusivity * use intersection types rather than extra interfaces * chore: use toString rather than Buffer * add node types to browser resolve Co-authored-by: Jordan Harband <ljharb@gmail.com>

@SimenB

…` by @SimenB * fix(resolve): add readPackage(Sync) * Update resolve-tests.ts * Update types/resolve/index.d.ts Co-authored-by: Jordan Harband <ljharb@gmail.com> * maybe fix ci * use never to force mutual exclusivity * use intersection types rather than extra interfaces * chore: use toString rather than Buffer * add node types to browser resolve Co-authored-by: Jordan Harband <ljharb@gmail.com>

fix(resolve): add readPackage(Sync)

910670f

typescript-bot added Critical package Untested Change This PR does not touch tests labels Feb 11, 2021

typescript-bot added this to Waiting for Code Reviews in New Pull Request Status Board Feb 11, 2021

SimenB commented Feb 11, 2021

View reviewed changes

typescript-bot moved this from Waiting for Code Reviews to Needs Maintainer Review in New Pull Request Status Board Feb 11, 2021

typescript-bot added the The CI failed When GH Actions fails label Feb 11, 2021

typescript-bot moved this from Needs Maintainer Review to Needs Author Action in New Pull Request Status Board Feb 11, 2021

Update resolve-tests.ts

d1aae00

typescript-bot added Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. and removed The CI failed When GH Actions fails Untested Change This PR does not touch tests labels Feb 11, 2021

typescript-bot moved this from Needs Author Action to Waiting for Code Reviews in New Pull Request Status Board Feb 11, 2021

typescript-bot added the The CI failed When GH Actions fails label Feb 11, 2021

typescript-bot moved this from Waiting for Code Reviews to Needs Author Action in New Pull Request Status Board Feb 11, 2021

SimenB commented Feb 11, 2021

View reviewed changes

ljharb approved these changes Feb 11, 2021

View reviewed changes

types/resolve/index.d.ts Show resolved Hide resolved

typescript-bot added the Other Approved This PR was reviewed and signed-off by a community member. label Feb 11, 2021

Update types/resolve/index.d.ts

0d37468

Co-authored-by: Jordan Harband <ljharb@gmail.com>

typescript-bot removed the Other Approved This PR was reviewed and signed-off by a community member. label Feb 11, 2021

maybe fix ci

30e9b28

typescript-bot added the Edits Owners This PR adds or removes owners label Feb 11, 2021

rbuckton approved these changes Feb 13, 2021

View reviewed changes

typescript-bot added Maintainer Approved Self Merge This PR can now be self-merged by the PR author or an owner labels Feb 13, 2021

typescript-bot moved this from Needs Maintainer Review to Waiting for Author to Merge in New Pull Request Status Board Feb 13, 2021

SimenB added 2 commits February 13, 2021 09:23

chore: use toString rather than Buffer

3e4ee0a

add node types to browser resolve

faf0de9

typescript-bot added Edits multiple packages and removed Maintainer Approved Other Approved This PR was reviewed and signed-off by a community member. Self Merge This PR can now be self-merged by the PR author or an owner labels Feb 13, 2021

typescript-bot moved this from Waiting for Author to Merge to Waiting for Code Reviews in New Pull Request Status Board Feb 13, 2021

typescript-bot moved this from Waiting for Code Reviews to Needs Maintainer Review in New Pull Request Status Board Feb 13, 2021

ljharb approved these changes Feb 13, 2021

View reviewed changes

typescript-bot added the Other Approved This PR was reviewed and signed-off by a community member. label Feb 13, 2021

orta approved these changes Feb 18, 2021

View reviewed changes

typescript-bot added Maintainer Approved Self Merge This PR can now be self-merged by the PR author or an owner labels Feb 18, 2021

typescript-bot moved this from Needs Maintainer Review to Waiting for Author to Merge in New Pull Request Status Board Feb 18, 2021

typescript-bot moved this from Waiting for Author to Merge to Recently Merged in New Pull Request Status Board Feb 18, 2021

typescript-bot merged commit 87a4155 into DefinitelyTyped:master Feb 18, 2021

SimenB deleted the patch-2 branch February 18, 2021 16:52

typescript-bot removed this from Recently Merged in New Pull Request Status Board Feb 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(resolve): add `readPackage(Sync)` #51182

fix(resolve): add `readPackage(Sync)` #51182

SimenB commented Feb 11, 2021

typescript-bot commented Feb 11, 2021 •

edited

typescript-bot commented Feb 11, 2021 •

edited

SimenB Feb 11, 2021

typescript-bot commented Feb 11, 2021

typescript-bot commented Feb 11, 2021

typescript-bot commented Feb 11, 2021

SimenB Feb 11, 2021

rbuckton Feb 13, 2021

rbuckton Feb 13, 2021

ljharb Feb 13, 2021

SimenB Feb 13, 2021

ljharb left a comment

SimenB commented Feb 11, 2021

typescript-bot commented Feb 11, 2021

typescript-bot commented Feb 11, 2021

typescript-bot commented Feb 13, 2021

typescript-bot commented Feb 13, 2021

SimenB commented Feb 18, 2021

typescript-bot commented Feb 18, 2021

SimenB commented Feb 18, 2021

typescript-bot commented Feb 18, 2021

fix(resolve): add readPackage(Sync) #51182

fix(resolve): add readPackage(Sync) #51182

Conversation

SimenB commented Feb 11, 2021

typescript-bot commented Feb 11, 2021 • edited

2 packages in this PR

Code Reviews

Status

typescript-bot commented Feb 11, 2021 • edited

SimenB Feb 11, 2021

Choose a reason for hiding this comment

typescript-bot commented Feb 11, 2021

typescript-bot commented Feb 11, 2021

typescript-bot commented Feb 11, 2021

SimenB Feb 11, 2021

Choose a reason for hiding this comment

rbuckton Feb 13, 2021

Choose a reason for hiding this comment

rbuckton Feb 13, 2021

Choose a reason for hiding this comment

ljharb Feb 13, 2021

Choose a reason for hiding this comment

SimenB Feb 13, 2021

Choose a reason for hiding this comment

ljharb left a comment

Choose a reason for hiding this comment

SimenB commented Feb 11, 2021

typescript-bot commented Feb 11, 2021

typescript-bot commented Feb 11, 2021

typescript-bot commented Feb 13, 2021

typescript-bot commented Feb 13, 2021

SimenB commented Feb 18, 2021

typescript-bot commented Feb 18, 2021

SimenB commented Feb 18, 2021

typescript-bot commented Feb 18, 2021

fix(resolve): add `readPackage(Sync)` #51182

fix(resolve): add `readPackage(Sync)` #51182

typescript-bot commented Feb 11, 2021 •

edited

typescript-bot commented Feb 11, 2021 •

edited