Skip to content

Fix node / webpack-env compatibility - #47685

Closed
sandersn wants to merge 4 commits into
masterfrom
fix-node-webpack-env-compatibility
Closed

sandersn wants to merge 4 commits into
masterfrom
fix-node-webpack-env-compatibility

Conversation

@sandersn

Copy link
Copy Markdown
Contributor

Update webpack-env to be compatible with node again.

Not sure what broke, but node's had a lot of updates recently, so it was probably that.

1. TS 4.1 no longer has a built-in type named 'Position'.
2. TS 4.1 intentionally sets the global `name: void` to make it less
usable.

This flushed out a few bugs.
Not sure what broke, but node's had a lot of updates recently, so it was
probably that.
@typescript-bot

typescript-bot commented Sep 16, 2020

Copy link
Copy Markdown
Contributor

@sandersn Thank you for submitting this PR!

This is a live comment which I will keep updated.

This PR doesn't modify any tests, so it's hard to know what's being fixed, and your changes might regress in the future. Have you considered adding tests to cover the change you're making? Including tests allows this PR to be merged by yourself and the owners of this module. This can potentially save days of time for you.

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.

Status

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

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": 47685,
  "author": "sandersn",
  "owners": [
    "use-strict",
    "rhonsby"
  ],
  "dangerLevel": "ScopedAndUntested",
  "headCommitAbbrOid": "59bfcf0",
  "headCommitOid": "59bfcf0d85a971d13fce594c754919e23bdbe41f",
  "mergeIsRequested": false,
  "stalenessInDays": 0,
  "lastPushDate": "2020-09-16T23:29:05.000Z",
  "lastCommentDate": "2020-09-16T23:29:05.000Z",
  "maintainerBlessed": false,
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/47685/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"
    }
  ],
  "hasDismissedReview": false,
  "ciResult": "pass",
  "reviewersWithStaleReviews": [],
  "approvalFlags": 0,
  "isChangesRequested": false
}

@typescript-bot

typescript-bot commented Sep 16, 2020

Copy link
Copy Markdown
Contributor

🔔 @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 typescript-bot added Untested Change This PR does not touch tests and removed Edits multiple packages labels Sep 16, 2020
@typescript-bot

Copy link
Copy Markdown
Contributor

👋 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 #47685 diff
Batch compilation
Memory usage (MiB) 35.9 35.7 -0.5%
Type count 2272 2272 0%
Assignability cache size 118 118 0%
Language service
Samples taken 119 119 0%
Identifiers in tests 119 119 0%
getCompletionsAtPosition
    Mean duration (ms) 95.1 94.2 -0.9%
    Mean CV 18.0% 19.0%
    Worst duration (ms) 128.3 131.0 +2.1%
    Worst identifier Error Error
getQuickInfoAtPosition
    Mean duration (ms) 94.2 95.4 +1.3%
    Mean CV 18.2% 18.6% +2.2%
    Worst duration (ms) 122.2 132.8 +8.7%
    Worst identifier disposeHandler disposeHandler

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

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

Copy link
Copy Markdown
Contributor Author

duplicate of #47516

@sandersn sandersn closed this Sep 17, 2020
@sandersn
sandersn deleted the fix-node-webpack-env-compatibility branch September 17, 2020 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Critical package Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. Untested Change This PR does not touch tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants