Support multiple minor versions in parallel #723

alloy · 2020-01-01T16:00:03Z

Regarding DefinitelyTyped/DefinitelyTyped#40923
Supersedes #717

Summary

Some packages, such as react-native, have been and will be < v1 for a while. Especially in the < v1 case, according to semver any v0.x version can break API compatibility. As such, it would be great if we could maintain minor versions in parallel, just like we already can with major versions.

After getting my bearings in the repo, I ended up adjusting the version directory validation to allow for minor versions and record that directory name in the definitions.json file so it can be used when generating packages.

Example run

$ npm run build
$ npm run clean
$ npm run parse
$ npm run check
$ npm run calculate-versions
$ npm run generate -- --single react-native
$ ack '"version":' ./output

output/react-native/package.json
3:    "version": "0.62.0",

output/react-native v0.61/package.json
3:    "version": "0.61.0",

output/react-native v0.60/package.json
3:    "version": "0.60.28",

Test plan

I’ve tried to add tests around code I’ve changed as much as possible and I’ve performed local runs of the steps that can be ran locally (as shown above). However, I do think this requires a handheld run by somebody that knows this project more intimately to ensure all the steps after generating packages work as expected.

alloy · 2020-01-01T16:06:59Z

cc @orta @acoates-ms

README.md

src/lib/definition-parser.ts

README.md

alloy · 2020-01-03T15:56:25Z

src/generate-packages.ts

@@ -39,7 +39,7 @@ export default async function generatePackages(dt: FS, allPackages: AllPackages,
        if (tgz) {
            await writeTgz(pkg.outputDirectory, `${pkg.outputDirectory}.tgz`);
        }
-        log(` * ${pkg.libraryName}`);
+        log(` * ${pkg.desc}`);


Adds the version of the package that’s being generated, rather than simply repeating the package name multiple times.

alloy · 2020-01-03T16:09:14Z

src/tester/test-runner.ts

@@ -282,22 +282,22 @@ async function runCommand(log: LoggerWithErrors, cwd: string | undefined, cmd: s

 /** Returns all immediate subdirectories of the root directory that have changed. */
 export function gitChanges(diffs: GitDiff[]): PackageId[] {
-    const changedPackages = new Map<string, Set<DependencyVersion>>();
+    const changedPackages = new Map<string, Map<string, DependencyVersion>>();


Now that DependencyVersion is no longer a single primitive, but rather an object, I had to replace the usage of Set…

alloy · 2020-01-03T16:09:42Z

src/tester/test-runner.ts


    for (const diff of diffs) {
        const dep = getDependencyFromFile(diff.file);
        if (dep) {
            const versions = changedPackages.get(dep.name);
            if (!versions) {
-                changedPackages.set(dep.name, new Set([dep.majorVersion]));
+                changedPackages.set(dep.name, new Map([[formatDependencyVersion(dep.version), dep.version]]));


…so now it’s keyed by a string representation of the version.

alloy · 2020-01-03T16:19:03Z

@sandersn @orta Alright, this should now be good to review.

orta

Looks good!

When I've tried to run master on my laptop I'm getting issues, can you check the diff for the results of npm run parse on this PR vs master to validate that the additional finer grain server checking hasn't accidentally changed the parsed representation overall?

That said, looks good to me

README.md

orta · 2020-01-06T20:41:22Z

package.json

+    "make-server-run": "node -r source-map-support/register bin/make-server-run.js",
+    "make-production-server-run": "node -r source-map-support/register bin/make-server-run.js --remote",
+    "test-tsNext": "node -r source-map-support/register bin/tester/test.js --all --tsNext",
+    "code-owners": "node -r source-map-support/register bin/code-owners.js",
    "push-production": "npm run build && git checkout production && git merge master && npm run build && git add bin && git commit -m \"Update bin\" && git push -u origin production"


…ck symbolication

alloy · 2020-01-07T20:21:33Z

@orta Rebased against master and still all seems to work fine for me 👍

alloy · 2020-01-07T20:22:24Z

@acoates-ms Unsure in how far you were planning on using your DT branch, but I had to make one small extra change DefinitelyTyped/DefinitelyTyped@5f1995c

acoates-ms · 2020-01-07T20:57:37Z

@alloy, The only other thing to check was that any changes to master since I made the change get copied into all the directories. -- Other than that, I was hoping to revive the PR once this one lands. (You are more than welcome to do that yourself though)

alloy · 2020-01-08T11:26:04Z

@acoates-ms Cool, I’ll pick that up once this lands 👍

sandersn

Overall the code looks good. There's a lot there but it's well-tested.

I'm on vacation early next week so I'll test the change manually then. In the meantime, I had some minor wording suggestions and one small question.

src/lib/versions.ts

src/lib/packages.ts

sandersn · 2020-01-17T23:29:03Z

src/lib/definition-parser.test.ts

+
+                expect(() => {
+                    getTypingInfo("jquery", dt.pkgFS("jquery"));
+                }).toThrow("The latest version is 3.3, but a directory v3 exists.");


it would read better as

"The latest version of ${packageName} is 3.3, so the subdirectory 'v3' is not allowed since it applies to 3.*, which is not older than 3.3."

(Not sure about how to get across the idea of 3.* -- there might be a clearer way.)

I went with:

The latest version is 3.3, so the subdirectory 'v3' is not allowed; since it applies to any 3.* version, up to and including 3.3.

What do you think?

src/lib/versions.ts

src/lib/packages.ts

sandersn · 2020-01-18T00:09:18Z

src/lib/definition-parser.ts

-    oldMajorVersion: number | undefined,
-): TypingsDataRaw {
+    directoryVersion: TypingVersion | undefined,
+): Omit<TypingsDataRaw, "libraryVersionDirectoryName"> {


why do we need to omit libraryVersionDirectoryName here?

libraryVersionDirectoryName is not determined in this function. I could have passed the value in, only to include it in the object and return it, but that felt like bolting it on. I can go either way, if you have a strong preference.

Co-Authored-By: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com>

alloy · 2020-01-28T20:16:55Z

Alright, addressed feedback 👍

alloy · 2020-02-12T15:29:08Z

@sandersn Could I get another review?

sandersn · 2020-02-14T01:13:10Z

@alloy I'm about to quit for the day so I'm going to publish in the morning, around 9 AM PST (-8 GMT).

sandersn · 2020-02-14T17:15:08Z

This is published and running on CI. I'll keep an eye out to see how it goes.

alloy · 2020-02-14T18:11:01Z

Ace 🙌 Thanks!

alloy · 2020-02-14T20:56:14Z

Looks like the fallout of this change thus far is:

~~Incorrect versions reported in CODEOWNERS 🤖 CODEOWNERS has changed DefinitelyTyped/DefinitelyTyped#42377~~ Fixed in Don't double-prefix version directory name #741
~~CI for packages with version dirs fail.~~ Also fixed by Don't double-prefix version directory name #741
The benchmark CI jobs failing (e.g. this one). Will follow-up on this PR [WIP] Fixes related to types-publisher now supporting multiple minor versions in parallel andrewbranch/definitely-not-slow#115

alloy requested review from RyanCavanaugh and sandersn as code owners January 1, 2020 16:00

alloy force-pushed the support-multiple-minor-versions branch from 4e29cdd to 60edd1a Compare January 1, 2020 16:05

alloy commented Jan 1, 2020

View reviewed changes

README.md Show resolved Hide resolved

src/lib/definition-parser.ts Outdated Show resolved Hide resolved

alloy commented Jan 3, 2020

View reviewed changes

README.md Show resolved Hide resolved

alloy commented Jan 3, 2020

View reviewed changes

alloy changed the title ~~[WIP] Support multiple minor versions in parallel~~ Support multiple minor versions in parallel Jan 3, 2020

orta mentioned this pull request Jan 6, 2020

Support majors as minors in folders #717

Closed

orta approved these changes Jan 6, 2020

View reviewed changes

alloy added 17 commits January 7, 2020 21:17

[test] Make DT mock mutable

a8f539b

[definition-parser] Add tests for current behaviour

1c44a5d

[definition-parser] Support minor version directories

6717045

[package] Update ts-jest

3522718

[test] Fix failure reporting by not symbolicating errors at runtime.

f8caef3

[README] Prefer npm scripts because those also enable runtime callsta…

35f2b6e

…ck symbolication

[definition-parser] Cleanup

cc30084

[package] Remove dependency on semver pkg again

1108693

[packages] Cleanup

c54f0c7

[packages] Unify and update docs on definitions data

d552716

[definition-parser] Record old minor version path mappings

69420f1

[packages] Change DependencyVersion to consider minor versions

1395a78

[packages] Take minor versions into consideration

3ed40b2

[tslint] Make green

33956c9

[README] Update example snippet from definitions.json

55b89e8

[definition-parser] Cleanup TODOs

5912189

[definition-parser] Get rid of unneeded data

1cc0d83

Cleanup more comments

284824c

alloy force-pushed the support-multiple-minor-versions branch from c040652 to 284824c Compare January 7, 2020 20:21

sandersn self-assigned this Jan 17, 2020

sandersn reviewed Jan 18, 2020

View reviewed changes

alloy and others added 4 commits January 28, 2020 16:53

Doc fixes.

e9ab043

Co-Authored-By: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com>

[versions] Simplify code

67470c6

More doc fixes

480e944

[definition-parser] Improve error output

78e61df

sandersn approved these changes Feb 14, 2020

View reviewed changes

sandersn merged commit 24c6039 into microsoft:master Feb 14, 2020

alloy mentioned this pull request Feb 14, 2020

🤖 CODEOWNERS has changed DefinitelyTyped/DefinitelyTyped#42377

Closed

alloy mentioned this pull request Feb 14, 2020

[WIP] Fixes related to types-publisher now supporting multiple minor versions in parallel andrewbranch/definitely-not-slow#115

Closed

alloy mentioned this pull request Mar 27, 2020

Support parallel minor versions microsoft/dtslint#282

Merged

sandersn mentioned this pull request Jul 30, 2021

Allow pre-1.0 /v* subdirectories. microsoft/dtslint#338

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multiple minor versions in parallel #723

Support multiple minor versions in parallel #723

alloy commented Jan 1, 2020 •

edited

Loading

alloy commented Jan 1, 2020

alloy Jan 3, 2020

alloy Jan 3, 2020 •

edited

Loading

alloy Jan 3, 2020

alloy commented Jan 3, 2020

orta left a comment

orta Jan 6, 2020

alloy commented Jan 7, 2020

alloy commented Jan 7, 2020

acoates-ms commented Jan 7, 2020

alloy commented Jan 8, 2020

sandersn left a comment

sandersn Jan 17, 2020

alloy Jan 28, 2020

sandersn Jan 18, 2020

alloy Jan 28, 2020 •

edited

Loading

alloy commented Jan 28, 2020

alloy commented Feb 12, 2020

sandersn commented Feb 14, 2020

sandersn commented Feb 14, 2020

alloy commented Feb 14, 2020

alloy commented Feb 14, 2020 •

edited

Loading

Support multiple minor versions in parallel #723

Support multiple minor versions in parallel #723

Conversation

alloy commented Jan 1, 2020 • edited Loading

Summary

Example run

Test plan

alloy commented Jan 1, 2020

alloy Jan 3, 2020

Choose a reason for hiding this comment

alloy Jan 3, 2020 • edited Loading

Choose a reason for hiding this comment

alloy Jan 3, 2020

Choose a reason for hiding this comment

alloy commented Jan 3, 2020

orta left a comment

Choose a reason for hiding this comment

orta Jan 6, 2020

Choose a reason for hiding this comment

alloy commented Jan 7, 2020

alloy commented Jan 7, 2020

acoates-ms commented Jan 7, 2020

alloy commented Jan 8, 2020

sandersn left a comment

Choose a reason for hiding this comment

sandersn Jan 17, 2020

Choose a reason for hiding this comment

alloy Jan 28, 2020

Choose a reason for hiding this comment

sandersn Jan 18, 2020

Choose a reason for hiding this comment

alloy Jan 28, 2020 • edited Loading

Choose a reason for hiding this comment

alloy commented Jan 28, 2020

alloy commented Feb 12, 2020

sandersn commented Feb 14, 2020

sandersn commented Feb 14, 2020

alloy commented Feb 14, 2020

alloy commented Feb 14, 2020 • edited Loading

alloy commented Jan 1, 2020 •

edited

Loading

alloy Jan 3, 2020 •

edited

Loading

alloy Jan 28, 2020 •

edited

Loading

alloy commented Feb 14, 2020 •

edited

Loading