Skip to content

Try to get minTypeScriptVersion-compatible dependencies#121

Merged
sandersn merged 2 commits intomicrosoft:masterfrom
jablko:patch-20
Oct 13, 2020
Merged

Try to get minTypeScriptVersion-compatible dependencies#121
sandersn merged 2 commits intomicrosoft:masterfrom
jablko:patch-20

Conversation

@jablko
Copy link
Copy Markdown
Contributor

@jablko jablko commented Sep 22, 2020

We'll be testing the types with minTypeScriptVersion (modulo onlyTestTsNext) so make an effort to get compatible external dependencies by adding the --tag ts${minTypeScriptVersion} option to npm install. If a dependency provides such a tag then we'll get that compatible version. If not, well, we'll continue to get the current behavior.

This adds the --tag option to prepareAffectedPackages() (trivial) and prepareAllPackages(), which previously used separate logic to crawl the types directory and bypassed @definitelytyped/definitions-parser. Adding the --tag option means reading the minTypeScriptVersion, so I made prepareAffectedPackages() and prepareAllPackages() share the same logic. I think this is desirable: You should get the same per-package behavior regardless of onlyRunAffectedPackages?

Comment on lines +23 to +24
const dt = await getDefinitelyTyped(options, log);
await parseDefinitions(dt, nProcesses ? { definitelyTypedPath, nProcesses } : undefined, log);
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.

This is expensive and unnecessary when testing all packages, which is why it wasn’t here before.

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.

👍 That line (parseDefinitions()) is easy enough to ax, however it looks like it's needed in order to get the minTypeScriptVersion of each package?

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.

Ah, I see. If that’s the case then it’s probably worth it. From the description I thought you were consolidating just for code reuse, so I wanted to be clear that there’s a pretty big performance tradeoff.

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.

I could get the minTypeScriptVersion a lot more efficiently than by calling parseDefinitions(), but on the other hand testing all packages is already an expensive operation. Like off the top of my head it currently takes about a couple hours, of which +/- 60 seconds is spent in parseDefinitions()? How does this optimization pay off?

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.

Yeah, you’re right. My bad 👍

@andrewbranch
Copy link
Copy Markdown
Member

I want to make sure this makes sense to @sandersn first

@jablko
Copy link
Copy Markdown
Contributor Author

jablko commented Sep 22, 2020

Absolutely, thanks for your help 👍

@sandersn sandersn merged commit 2abeadf into microsoft:master Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants