[webpack-env] make __WebpackModuleApi.Module compatible with NodeJS.Module by jamescdavis · Pull Request #47516 · DefinitelyTyped/DefinitelyTyped

jamescdavis · 2020-09-11T20:58:40Z

A recent update to node types made __WebpackModuleApi.Module incompatible with NodeJS.Module. Importing the node types in the test reveals this issue and ensures compatibility.

Fixes [@types/node] error TS2430: Interface 'NodeJS.Module' incorrectly extends interface '__WebpackModuleApi.Module'. #47602
Closes [@types/node] Removed invalid type in globals.d.ts for parent conflicting with webpack #47607

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: [node] Update some apis #47485
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-09-11T20:59:19Z

@jamescdavis Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

webpack-env 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 or DT maintainers

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 47516,
  "author": "jamescdavis",
  "owners": [
    "use-strict",
    "rhonsby"
  ],
  "dangerLevel": "ScopedAndTested",
  "headCommitAbbrOid": "31ed0f6",
  "headCommitOid": "31ed0f66bdb26883f319f61ab65f16b5db948acf",
  "mergeIsRequested": false,
  "stalenessInDays": 0,
  "lastPushDate": "2020-09-12T16:40:37.000Z",
  "lastCommentDate": "2020-09-17T14:50:24.000Z",
  "maintainerBlessed": true,
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/47516/files",
  "hasMergeConflict": false,
  "authorIsOwner": false,
  "isFirstContribution": false,
  "popularityLevel": "Critical",
  "newPackages": [],
  "packages": [
    "webpack-env"
  ],
  "files": [
    {
      "path": "types/webpack-env/index.d.ts",
      "kind": "definition",
      "package": "webpack-env"
    },
    {
      "path": "types/webpack-env/webpack-env-tests.ts",
      "kind": "test",
      "package": "webpack-env"
    }
  ],
  "hasDismissedReview": false,
  "ciResult": "pass",
  "lastReviewDate": "2020-09-12T17:04:49.000Z",
  "reviewersWithStaleReviews": [],
  "approvalFlags": 1,
  "isChangesRequested": false
}

typescript-bot · 2020-09-11T20:59:20Z

🔔 @use-strict @rhonsby — 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-09-11T21:04:38Z

👋 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	#47516	diff
Batch compilation
Memory usage (MiB)	33.7	33.6	-0.3%
Type count	2257	2257	0%
Assignability cache size	114	114	0%

Language service
Samples taken	119	119	0%
Identifiers in tests	119	119	0%
`getCompletionsAtPosition`
Mean duration (ms)	79.7	78.4	-1.7%
Mean CV	20.7%	20.9%
Worst duration (ms)	120.9	101.0	-16.4%
Worst identifier	hot	err
`getQuickInfoAtPosition`
Mean duration (ms)	78.4	79.3	+1.3%
Mean CV	18.6%	21.5%	+15.8%
Worst duration (ms)	105.4	101.3	-3.9%
Worst identifier	ensure	outdatedModules

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

typescript-bot · 2020-09-11T21:20:34Z

Updated numbers for you here from b0875df.

Comparison details 📊

	master	#47516	diff
Batch compilation
Memory usage (MiB)	33.5	66.7	+99.2%
Type count	2257	9080	+302%
Assignability cache size	114	1628	+1328% 🚨

Language service
Samples taken	119	119	0%
Identifiers in tests	119	119	0%
`getCompletionsAtPosition`
Mean duration (ms)	80.0	352.7	+341.0% 🚨
Mean CV	20.7%	10.1%
Worst duration (ms)	108.7	421.1	+287.3% 🚨
Worst identifier	hot	Error
`getQuickInfoAtPosition`
Mean duration (ms)	80.5	348.3	+332.4% 🚨
Mean CV	21.1%	9.4%	-55.6%
Worst duration (ms)	111.8	428.9	+283.7% 🚨
Worst identifier	disposeHandler	e

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.

jamescdavis · 2020-09-11T21:21:21Z

        filename: string;
        loaded: boolean;
-        parent: NodeModule | null;
+        parent: NodeModule | null | undefined;


I could have used

Suggested change

parent: NodeModule | null | undefined;

parent?: NodeModule | null;

of course, but I was going for exact parity with the node types:

DefinitelyTyped/types/node/ts3.1/globals.d.ts

Line 580 in 1c2240f

parent: Module | null | undefined;

jamescdavis · 2020-09-11T21:37:15Z

I'm assuming the perf change is only due to the import of the node types in the test.

Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com>

typescript-bot · 2020-09-12T16:45:09Z

@ExE-Boss 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-12T16:48:57Z

Updated numbers for you here from dbb655b.

Comparison details 📊

	master	#47516	diff
Batch compilation
Memory usage (MiB)	33.5	63.6	+90.1%
Type count	2257	9080	+302%
Assignability cache size	114	1628	+1328% 🚨

Language service
Samples taken	119	119	0%
Identifiers in tests	119	119	0%
`getCompletionsAtPosition`
Mean duration (ms)	78.1	334.3	+327.9% 🚨
Mean CV	21.3%	10.8%
Worst duration (ms)	105.4	403.5	+282.9% 🚨
Worst identifier	hot	hot
`getQuickInfoAtPosition`
Mean duration (ms)	77.0	331.2	+330.1% 🚨
Mean CV	20.5%	9.4%	-54.1%
Worst duration (ms)	99.5	400.7	+302.6% 🚨
Worst identifier	disposeHandler	status

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.

Remove once DefinitelyTyped/DefinitelyTyped#47516 is merged

dep · 2020-09-14T13:30:17Z

How can I get these @typescript-bot magical updates in my own project?

ExE-Boss · 2020-09-14T13:44:27Z

@dep-dt Ask in https://github.com/DefinitelyTyped/dt-mergebot

jamescdavis · 2020-09-15T15:56:08Z

@use-strict @rhonsby looks like DT maintainers would like you to review. 🙏

ExE-Boss · 2020-09-15T20:57:48Z

This fixes issue #47602 and closes PR #47607, so this should be placed in the PR description:

- Fixes <https://github.com/DefinitelyTyped/DefinitelyTyped/issues/47602>
- Closes <https://github.com/DefinitelyTyped/DefinitelyTyped/pull/47607>

ulrikstrid · 2020-09-16T08:44:18Z

Is it possible to use this without a release to npm?

olyop · 2020-09-16T09:00:04Z

Is it possible to use this without a release to npm?

Since its just a typescript issue, you can manually edit file and add the undefined option in the node_modules folder.

ulrikstrid · 2020-09-16T09:06:58Z

@olyop Not really feasible in CI 😂 hopefully this lands soon, or is there some workaround I can do with a flag in tsconfig.json?

andrevmatos · 2020-09-16T10:09:52Z

@ulrikstrid pin @types/node@14.10.0 devDependency until this gets merged. The offending change was published as 14.10.1.

mmso · 2020-09-17T09:04:57Z

Will this still get merged in case a maintainer review never happens?

jamescdavis · 2020-09-17T14:50:24Z

Will this still get merged in case a maintainer review never happens?

@sandersn?

jamescdavis · 2020-09-17T19:02:38Z

❤️ @sandersn

jamescdavis · 2020-09-17T19:04:53Z

This was released as @types/webpack-env@1.15.3.

…odule (DefinitelyTyped#47516) * fix: make __WebpackModuleApi.Module compatible with NodeJS.Module * test: import node types in test * test: use reference instead of import Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com> Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com>

fix: make __WebpackModuleApi.Module compatible with NodeJS.Module

5638439

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

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

test: import node types in test

f5ea1af

typescript-bot removed the Untested Change This PR does not touch tests label Sep 11, 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 11, 2020

jamescdavis commented Sep 11, 2020

View reviewed changes

Semigradsky referenced this pull request Sep 12, 2020

[node] Update some apis (#47485)

2e8ce3e

ExE-Boss reviewed Sep 12, 2020

View reviewed changes

Comment thread types/webpack-env/webpack-env-tests.ts Outdated

davidje13 mentioned this pull request Sep 12, 2020

fix(45927): fix missing path for Module in v12 #45940

Merged

3 tasks

test: use reference instead of import

31ed0f6

Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com>

ExE-Boss approved these changes Sep 12, 2020

View reviewed changes

typescript-bot added the Other Approved This PR was reviewed and signed-off by a community member. label Sep 12, 2020

proton-ci pushed a commit to ProtonMail/proton-shared that referenced this pull request Sep 14, 2020

Temporarily lock @types/node

e1d9015

Remove once DefinitelyTyped/DefinitelyTyped#47516 is merged

nmquebb mentioned this pull request Sep 15, 2020

typescript template broken by default: Types of property 'parent' are incompatible preactjs/preact-cli#1425

Closed

andrevmatos mentioned this pull request Sep 15, 2020

dependencies: Update all non-major dependencies raiden-network/light-client#2135

Merged

1 task

Flarna mentioned this pull request Sep 15, 2020

[@types/node] error TS2430: Interface 'NodeJS.Module' incorrectly extends interface '__WebpackModuleApi.Module'. #47602

Closed

4 tasks

euanmillar mentioned this pull request Sep 15, 2020

[@types/node] Removed invalid type in globals.d.ts for parent conflicting with webpack #47607

Closed

1 task

sandersn merged commit 3dc75e3 into DefinitelyTyped:master Sep 17, 2020

sandersn mentioned this pull request Sep 17, 2020

Fix node / webpack-env compatibility #47685

Closed

jamescdavis deleted the fix_webpack-env branch September 17, 2020 18:54

clomie mentioned this pull request Sep 21, 2020

Error when building fresh install clomie/gas-vue-typescript#23

Closed

ExE-Boss mentioned this pull request Sep 30, 2020

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

Merged

10 tasks

Semigradsky mentioned this pull request Oct 27, 2020

Type 'Module | null | undefined' is not assignable to type 'NodeModule | null' #49151

Closed

	parent: NodeModule \| null \| undefined;
	parent?: NodeModule \| null;

Conversation

jamescdavis commented Sep 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Sep 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1 package in this PR

Code Reviews

Status

Uh oh!

typescript-bot commented Sep 11, 2020

Uh oh!

typescript-bot commented Sep 11, 2020

Uh oh!

typescript-bot commented Sep 11, 2020

Uh oh!

jamescdavis Sep 11, 2020

Choose a reason for hiding this comment

Uh oh!

jamescdavis commented Sep 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

typescript-bot commented Sep 12, 2020

Uh oh!

typescript-bot commented Sep 12, 2020

Uh oh!

dep commented Sep 14, 2020

Uh oh!

ExE-Boss commented Sep 14, 2020

Uh oh!

jamescdavis commented Sep 15, 2020

Uh oh!

ExE-Boss commented Sep 15, 2020

Uh oh!

ulrikstrid commented Sep 16, 2020

Uh oh!

olyop commented Sep 16, 2020

Uh oh!

ulrikstrid commented Sep 16, 2020

Uh oh!

andrevmatos commented Sep 16, 2020

Uh oh!

mmso commented Sep 17, 2020

Uh oh!

jamescdavis commented Sep 17, 2020

Uh oh!

jamescdavis commented Sep 17, 2020

Uh oh!

jamescdavis commented Sep 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

jamescdavis commented Sep 11, 2020 •

edited

Loading

typescript-bot commented Sep 11, 2020 •

edited

Loading

jamescdavis commented Sep 11, 2020 •

edited

Loading