Skip to content

Add typescript dependency#45226

Merged
sandersn merged 5 commits intomasterfrom
add-typescript-dep
Jun 2, 2020
Merged

Add typescript dependency#45226
sandersn merged 5 commits intomasterfrom
add-typescript-dep

Conversation

@sandersn
Copy link
Copy Markdown
Contributor

@sandersn sandersn commented Jun 1, 2020

In preparation for merging microsoft/dtslint#295 and shipping a new version of dtslint that has typescript as a peer dependency.

@sandersn sandersn requested review from andrewbranch and orta June 1, 2020 23:48
package.json Outdated
}
},
"dependencies": {}
"dependencies": {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not devDependencies?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because DefinitelyTyped isn't on npm so nothing can take a dependency on it? But I guess it makes more sense there.

Copy link
Copy Markdown
Contributor

@Maxim-Mazurok Maxim-Mazurok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@andrewbranch
Copy link
Copy Markdown
Member

/azp run DefinitelyTyped.BenchmarkPR

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

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

webpack-sources/v*

Comparison details for webpack-sources/* 📊
master #45226 diff
Batch compilation
Memory usage (MiB) 66.4 66.3 -0.3%
Type count 9468 9437 0%
Assignability cache size 3281 3308 +1%
Language service
Samples taken 190 40 -79%
Identifiers in tests 190 40 -79%
getCompletionsAtPosition
    Mean duration (ms) 309.4 320.1 +3.5%
    Mean CV 11.7% 14.6%
    Worst duration (ms) 363.9 379.0 +4.2%
    Worst identifier sourceAndMap a
getQuickInfoAtPosition
    Mean duration (ms) 307.9 314.9 +2.2%
    Mean CV 11.2% 13.2% +18.5%
    Worst duration (ms) 366.8 413.3 +12.7%
    Worst identifier rawSource RawSource

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

webpack-sources/v*

Comparison details for webpack-sources/* 📊
master #45226 diff
Batch compilation
Memory usage (MiB) 66.4 65.1 -1.9%
Type count 9468 9437 0%
Assignability cache size 3281 3308 +1%
Language service
Samples taken 190 40 -79%
Identifiers in tests 190 40 -79%
getCompletionsAtPosition
    Mean duration (ms) 298.0 290.1 -2.6%
    Mean CV 11.7% 15.0%
    Worst duration (ms) 387.2 356.4 -7.9%
    Worst identifier source ConcatSource
getQuickInfoAtPosition
    Mean duration (ms) 296.5 290.9 -1.9%
    Mean CV 11.4% 16.1% +41.2%
    Worst duration (ms) 380.2 360.8 -5.1%
    Worst identifier node b

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 Jun 2, 2020
package.json Outdated
Comment on lines +29 to +30
"@definitelytyped/dtslint-runner": "latest",
"typescript": "next"
Copy link
Copy Markdown
Contributor

@ExE-Boss ExE-Boss Jun 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make the diff cleaner, I suggest putting typescript@next before @definitelytyped/dtslint‑runner, like in #45148:

Suggested change
"@definitelytyped/dtslint-runner": "latest",
"typescript": "next"
"typescript": "next",
"@definitelytyped/dtslint-runner": "latest"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I suggest using npx sort-package-json that will produce this:

    "devDependencies": {
        "@definitelytyped/dtslint-runner": "latest",
        "danger": "^10.1.1",
        "dtslint": "latest",
        "prettier": "^2.0.2",
        "typescript": "next"
    }

The reason is that package.json will be re-ordered anyway in case if a package will be added/removed using npm cli.

@sandersn sandersn merged commit a4f9041 into master Jun 2, 2020
@sandersn sandersn deleted the add-typescript-dep branch June 2, 2020 15:40
jjballano-qatium pushed a commit to jjballano-qatium/DefinitelyTyped that referenced this pull request Jun 16, 2020
* add typescript dependency

* move typescript to devDependencies

* try ts version that matches perf

* try ts@next again

* reorder devDeps
ngbrown pushed a commit to ngbrown-forks/DefinitelyTyped that referenced this pull request Jul 11, 2020
* add typescript dependency

* move typescript to devDependencies

* try ts version that matches perf

* try ts@next again

* reorder devDeps
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants