Skip to content

[@types/node] Fix ProcessEnv#52467

Closed
JIACHENG9 wants to merge 1 commit into
DefinitelyTyped:masterfrom
JIACHENG9:node/process
Closed

[@types/node] Fix ProcessEnv#52467
JIACHENG9 wants to merge 1 commit into
DefinitelyTyped:masterfrom
JIACHENG9:node/process

Conversation

@JIACHENG9
Copy link
Copy Markdown
Contributor

@JIACHENG9 JIACHENG9 commented Apr 20, 2021

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: <>

https://github.com/nodejs/node/blob/master/src/node_env_var.cc

if (env->options()->pending_deprecation && !value->IsString() &&
      !value->IsNumber() && !value->IsBoolean() &&
      env->EmitProcessEnvWarning())

https://nodejs.org/dist/latest-v15.x/docs/api/deprecations.html#DEP0104

When assigning a non-string property to process.env, the assigned value is implicitly converted to a string. This behavior is deprecated if the assigned value is not a string, boolean, or number. In the future, such assignment might result in a thrown error. Please convert the property to a string before assigning it to process.env.

So the three types of string, number, and boolean are allowed

@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Apr 20, 2021 •
edited
Loading

@JIACHENG9 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.

Status

  • ✅ No merge conflicts
  • ❌ Continuous integration tests have failed
  • 🕐 Most recent commit is approved by a DT maintainer

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": 52467,
  "author": "JIACHENG9",
  "headCommitOid": "835502191c514ab6871adcc720d2c9e8e66a0d54",
  "lastPushDate": "2021-04-20T06:06:54.000Z",
  "lastActivityDate": "2021-04-20T07:04:40.000Z",
  "maintainerBlessed": false,
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "node",
      "kind": "edit",
      "files": [
        {
          "path": "types/node/process.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/test/path.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v10/globals.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v12/globals.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/worker_threads.d.ts",
          "kind": "definition"
        }
      ],
      "owners": [
        "Microsoft",
        "DefinitelyTyped",
        "jkomyno",
        "alvis",
        "r3nya",
        "btoueg",
        "brunoscheufler",
        "smac89",
        "touffy",
        "DeividasBakanas",
        "eyqs",
        "Hannes-Magnusson-CK",
        "KSXGitHub",
        "hoo29",
        "kjin",
        "ajafff",
        "islishude",
        "mwiktorczyk",
        "mohsen1",
        "n-e",
        "galkin",
        "parambirs",
        "eps1lon",
        "SimonSchick",
        "ThomasdenH",
        "WilcoBakker",
        "wwwy3y3",
        "samuela",
        "kuehlein",
        "bhongy",
        "chyzwar",
        "trivikr",
        "nguymin4",
        "yoursunny",
        "qwelias",
        "ExE-Boss",
        "Ryan-Willpower",
        "peterblazejewicz",
        "addaleax",
        "JasonHK",
        "victorperin",
        "ZYSzys"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [],
  "ciResult": "fail",
  "ciUrl": "https://github.com/DefinitelyTyped/DefinitelyTyped/commit/835502191c514ab6871adcc720d2c9e8e66a0d54/checks?check_suite_id=2536416202"
}

@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Apr 20, 2021

🔔 @microsoft @DefinitelyTyped @jkomyno @alvis @r3nya @btoueg @BrunoScheufler @smac89 @Touffy @DeividasBakanas @eyqs @Hannes-Magnusson-CK @KSXGitHub @hoo29 @kjin @ajafff @islishude @mwiktorczyk @mohsen1 @n-e @galkin @parambirs @eps1lon @SimonSchick @ThomasdenH @WilcoBakker @wwwy3y3 @samuela @kuehlein @bhongy @chyzwar @trivikr @nguymin4 @yoursunny @qwelias @ExE-Boss @Ryan-Willpower @peterblazejewicz @addaleax @JasonHK @victorperin @ZYSzys — 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
Copy link
Copy Markdown
Contributor

typescript-bot commented Apr 20, 2021

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

node/v14.14

Comparison details for node/14.14 📊
master #52467 diff
Batch compilation
Memory usage (MiB) 134.9 139.8 +3.6%
Type count 25394 25396 0%
Assignability cache size 8819 8820 0%
Language service
Samples taken 46 46 0%
Identifiers in tests 46 46 0%
getCompletionsAtPosition
    Mean duration (ms) 842.7 858.0 +1.8%
    Mean CV 11.2% 11.2%
    Worst duration (ms) 1006.3 980.9 -2.5%
    Worst identifier b a
getQuickInfoAtPosition
    Mean duration (ms) 824.3 841.6 +2.1%
    Mean CV 10.2% 9.6%
    Worst duration (ms) 1023.5 959.1 -6.3%
    Worst identifier a ok
System information
Node version v14.16.1 v14.16.1
CPU count 2 2
CPU speed 2.294 GHz 2.294 GHz
CPU model Intel(R) Xeon(R) CPU E5-2673 v4 @ 2.30GHz Intel(R) Xeon(R) CPU E5-2673 v4 @ 2.30GHz
CPU Architecture x64 x64
Memory 6.8 GiB 6.8 GiB
Platform linux linux
Release 4.15.0-1111-azure 4.15.0-1111-azure

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

node/v14.14

Comparison details for node/14.14 📊
master #52467 diff
Batch compilation
Memory usage (MiB) 134.4 136.0 +1.2%
Type count 25394 25396 0%
Assignability cache size 8819 8820 0%
Language service
Samples taken 46 46 0%
Identifiers in tests 46 46 0%
getCompletionsAtPosition
    Mean duration (ms) 896.8 804.5 -10.3%
    Mean CV 11.6% 11.7%
    Worst duration (ms) 1061.7 950.0 -10.5%
    Worst identifier a b
getQuickInfoAtPosition
    Mean duration (ms) 872.0 789.2 -9.5%
    Mean CV 10.0% 12.2%
    Worst duration (ms) 1022.5 1035.7 +1.3%
    Worst identifier a a

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

node/v14.14

Comparison details for node/14.14 📊
master #52467 diff
Batch compilation
Memory usage (MiB) 135.6 133.9 -1.2%
Type count 25394 25396 0%
Assignability cache size 8819 8820 0%
Language service
Samples taken 46 46 0%
Identifiers in tests 46 46 0%
getCompletionsAtPosition
    Mean duration (ms) 784.7 835.1 +6.4%
    Mean CV 11.5% 11.2%
    Worst duration (ms) 899.4 966.9 +7.5%
    Worst identifier assert b
getQuickInfoAtPosition
    Mean duration (ms) 773.0 814.2 +5.3%
    Mean CV 11.6% 12.0%
    Worst duration (ms) 886.6 958.2 +8.1%
    Worst identifier a ok

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

@typescript-bot typescript-bot added Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. The CI failed When GH Actions fails labels Apr 20, 2021
@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Apr 20, 2021

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

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

@peterblazejewicz
Copy link
Copy Markdown
Member

peterblazejewicz commented Apr 20, 2021

@JIACHENG9 appreciate the effort! We cannot do this yet I believe. Please see comments in most recent similar PR:
#51273
If implemented, it would need to be behind versioned definitions, just to support this new feature in TS (when released):
microsoft/TypeScript#42425

@JIACHENG9
Copy link
Copy Markdown
Contributor Author

JIACHENG9 commented Apr 20, 2021

@peterblazejewicz ok,this pr seems to affect many other libs

@JIACHENG9 JIACHENG9 closed this Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Critical package Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. The CI failed When GH Actions fails

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@JIACHENG9 @typescript-bot @peterblazejewicz