feat(pnpapi): Add `process.versions.pnp` #47107

ExE-Boss · 2020-08-28T13:35:35Z

Please fill in this template.

Use a meaningful title for the pull request. Include the name of the package modified.
Test the change in your own code. (Compile and run.)
Add or edit tests to reflect the change. (Run with npm test.)
Follow the advice from the readme.
Avoid common mistakes.
Run npm run lint package-name (or tsc if no tslint.json is present).

If changing an existing definition:

Provide a URL to documentation or source code which provides context for the suggested changes: fix(node): add process.versions.pnp property #46700 (comment) (https://yarnpkg.com/advanced/pnpapi#processversionspnp)
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.
Include tests for your changes
If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }. If for reason the any rule need to be disabled, disable it for that line using // tslint:disable-next-line [ruleName] and not for whole package so that the need for disabling can be reviewed.

typescript-bot · 2020-08-28T13:36:10Z

@ExE-Boss Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

pnpapi on npm, on unpkg

Code Reviews

Because you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it.

Status

✅ No merge conflicts
✅ Continuous integration tests have passed
✅ Most recent commit is approved by type definition owners, DT maintainers or others

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": 47107,
  "author": "ExE-Boss",
  "owners": [
    "arcanis"
  ],
  "dangerLevel": "ScopedAndTested",
  "headCommitAbbrOid": "f29b364",
  "headCommitOid": "f29b364f973ec6625a0d27e600e83303110ffafe",
  "mergeIsRequested": true,
  "stalenessInDays": 0,
  "lastPushDate": "2020-09-30T12:10:04.000Z",
  "reopenedDate": "2020-09-29T20:42:59.000Z",
  "lastCommentDate": "2020-09-30T19:59:27.000Z",
  "maintainerBlessed": false,
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/47107/files",
  "hasMergeConflict": false,
  "authorIsOwner": false,
  "isFirstContribution": false,
  "popularityLevel": "Well-liked by everyone",
  "newPackages": [],
  "packages": [
    "pnpapi"
  ],
  "files": [
    {
      "path": "types/pnpapi/index.d.ts",
      "kind": "definition",
      "package": "pnpapi"
    },
    {
      "path": "types/pnpapi/pnpapi-tests.ts",
      "kind": "test",
      "package": "pnpapi"
    }
  ],
  "hasDismissedReview": false,
  "ciResult": "pass",
  "lastReviewDate": "2020-09-30T19:15:10.000Z",
  "reviewersWithStaleReviews": [
    {
      "reviewedAbbrOid": "5ff2d86",
      "reviewer": "SimenB",
      "date": "2020-08-28T16:17:17Z"
    }
  ],
  "approvalFlags": 4,
  "isChangesRequested": false
}

typescript-bot · 2020-08-28T13:36:12Z

🔔 @arcanis — 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 · 2020-08-28T13:40:25Z

👋 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?

Comparison details 📊

	master	#47107	diff
Batch compilation
Memory usage (MiB)	36.4	36.1	-0.7%
Type count	2131	2132	0%
Assignability cache size	97	97	0%

Language service
Samples taken	25	30	+20%
Identifiers in tests	25	30	+20%
`getCompletionsAtPosition`
Mean duration (ms)	84.8	84.1	-0.8%
Mean CV	23.2%	21.2%
Worst duration (ms)	102.1	105.4	+3.2%
Worst identifier	pnp	getPackageInformation
`getQuickInfoAtPosition`
Mean duration (ms)	80.0	79.6	-0.5%
Mean CV	16.2%	16.6%	+2.7%
Worst duration (ms)	95.5	90.8	-4.9%
Worst identifier	resolution2	resolveUnqualified

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

SimenB · 2020-08-28T16:16:59Z

types/pnpapi/index.d.ts

+declare global {
+    namespace NodeJS {
+        interface ProcessVersions {
+            pnp: string;


Shouldn't it still be optional? Just loading the types doesn't mean it's pnp at runtime

Agreed 👍

amcasey · 2020-09-29T19:54:21Z

Closing as abandoned.

ExE-Boss · 2020-09-29T19:57:51Z

@amcasey

This isn’t abandoned, because it’s blocked on review from @arcanis.

arcanis · 2020-09-29T20:21:32Z

Sorry, I didn't see it before! @SimenB's comment should be addressed, then it's fine by me 😃

amcasey · 2020-09-29T20:42:58Z

@ExE-Boss My mistake. The comment from 30 days ago was unaddressed, so I assumed this was no longer interesting.

amcasey

Please address the feedback from @SimenB.

typescript-bot · 2020-09-29T20:44:20Z

@ExE-Boss One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. If you disagree with the reviewer's comments, you can "dismiss" the review using GitHub's review UI. Thank you!

Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>

ExE-Boss · 2020-09-30T12:13:03Z

types/pnpapi/pnpapi-tests.ts

@@ -1,3 +1,4 @@
+/// <reference types="node"/>


I’ve added @types/node to devDependencies so that changes to Node type definitions won’t cause breakage like with @types/webpack‑env: #47602 (PR #47516).

typescript-bot · 2020-09-30T12:14:42Z

@SimenB 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 · 2020-09-30T12:15:22Z

Updated numbers for you here from d883808.

Comparison details 📊

	master	#47107	diff
Batch compilation
Memory usage (MiB)	36.3	68.2	+87.5%
Type count	2151	9019	+319%
Assignability cache size	101	1616	+1500% 🚨

Language service
Samples taken	25	28	+12%
Identifiers in tests	25	28	+12%
`getCompletionsAtPosition`
Mean duration (ms)	80.5	335.5	+316.5% 🚨
Mean CV	21.7%	10.6%
Worst duration (ms)	107.0	391.1	+265.6% 🚨
Worst identifier	resolveUnqualified	versions
`getQuickInfoAtPosition`
Mean duration (ms)	77.2	335.0	+334.0% 🚨
Mean CV	19.8%	11.3%	-42.8%
Worst duration (ms)	89.7	371.4	+313.9% 🚨
Worst identifier	locator	locator

It looks like there are several metrics that changed quite a bit. You might want to take a look and make sure your changes won’t cause slow-downs for users consuming these types.

typescript-bot · 2020-09-30T19:15:46Z

@ExE-Boss Everything looks good here. Great job! I am ready to merge this PR 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! ❤️

(@arcanis: you can do this too.)

ExE-Boss · 2020-09-30T19:59:27Z

Ready to merge

typescript-bot · 2020-09-30T20:36:14Z

I just published @types/pnpapi@0.0.1 to npm.

ExE-Boss mentioned this pull request Aug 28, 2020

fix(node): add process.versions.pnp property #46700

Closed

10 tasks

typescript-bot added the Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. label Aug 28, 2020

ExE-Boss force-pushed the types/pnpapi/add-process-versions-pnp branch from 000f9c1 to 5ff2d86 Compare August 28, 2020 13:41

SimenB reviewed Aug 28, 2020

View reviewed changes

typescript-bot added the Abandoned This PR had no activity for a long time, and is considered abandoned label Sep 29, 2020

amcasey closed this Sep 29, 2020

amcasey reopened this Sep 29, 2020

typescript-bot removed the Abandoned This PR had no activity for a long time, and is considered abandoned label Sep 29, 2020

amcasey requested changes Sep 29, 2020

View reviewed changes

typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Sep 29, 2020

ExE-Boss force-pushed the types/pnpapi/add-process-versions-pnp branch 2 times, most recently from 0d57bb3 to f29b364 Compare September 30, 2020 12:10

feat(pnpapi): Add process.versions.pnp

f29b364

Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>

ExE-Boss commented Sep 30, 2020

View reviewed changes

typescript-bot removed the Revision needed This PR needs code changes before it can be merged. label Sep 30, 2020

typescript-bot added Perf: Worse typescript-bot determined that this PR has a negative impact on compilation performance. and removed Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. labels Sep 30, 2020

ExE-Boss requested a review from amcasey September 30, 2020 12:16

amcasey approved these changes Sep 30, 2020

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 Sep 30, 2020

typescript-bot merged commit 6851cef into DefinitelyTyped:master Sep 30, 2020

ExE-Boss deleted the types/pnpapi/add-process-versions-pnp branch September 30, 2020 20:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(pnpapi): Add `process.versions.pnp` #47107

feat(pnpapi): Add `process.versions.pnp` #47107

ExE-Boss commented Aug 28, 2020 •

edited

Loading

typescript-bot commented Aug 28, 2020 •

edited

Loading

typescript-bot commented Aug 28, 2020

typescript-bot commented Aug 28, 2020

SimenB Aug 28, 2020

arcanis Sep 29, 2020

amcasey commented Sep 29, 2020

ExE-Boss commented Sep 29, 2020 •

edited

Loading

arcanis commented Sep 29, 2020

amcasey commented Sep 29, 2020

amcasey left a comment

typescript-bot commented Sep 29, 2020

ExE-Boss Sep 30, 2020

typescript-bot commented Sep 30, 2020

typescript-bot commented Sep 30, 2020

typescript-bot commented Sep 30, 2020

ExE-Boss commented Sep 30, 2020

typescript-bot commented Sep 30, 2020

feat(pnpapi): Add process.versions.pnp #47107

feat(pnpapi): Add process.versions.pnp #47107

Conversation

ExE-Boss commented Aug 28, 2020 • edited Loading

typescript-bot commented Aug 28, 2020 • edited Loading

1 package in this PR

Code Reviews

Status

typescript-bot commented Aug 28, 2020

typescript-bot commented Aug 28, 2020

SimenB Aug 28, 2020

Choose a reason for hiding this comment

arcanis Sep 29, 2020

Choose a reason for hiding this comment

amcasey commented Sep 29, 2020

ExE-Boss commented Sep 29, 2020 • edited Loading

arcanis commented Sep 29, 2020

amcasey commented Sep 29, 2020

amcasey left a comment

Choose a reason for hiding this comment

typescript-bot commented Sep 29, 2020

ExE-Boss Sep 30, 2020

Choose a reason for hiding this comment

typescript-bot commented Sep 30, 2020

typescript-bot commented Sep 30, 2020

typescript-bot commented Sep 30, 2020

ExE-Boss commented Sep 30, 2020

typescript-bot commented Sep 30, 2020

feat(pnpapi): Add `process.versions.pnp` #47107

feat(pnpapi): Add `process.versions.pnp` #47107

ExE-Boss commented Aug 28, 2020 •

edited

Loading

typescript-bot commented Aug 28, 2020 •

edited

Loading

ExE-Boss commented Sep 29, 2020 •

edited

Loading