@reubenrybnik Thank you for submitting this PR!

This is a live comment which I will keep updated.

Code Reviews

Because you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it.

Status

• ✅ No merge conflicts
• ✅ Continuous integration tests have passed
• ✅ Most recent commit is approved by type definition owners, DT maintainers or others

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.

{
  "type": "info",
  "now": "-",
  "pr_number": 45748,
  "author": "reubenrybnik",
  "owners": [
    "ullisenmedia",
    "dpoetzsch",
    "jaeggerr",
    "flavionegrao",
    "wesleygrimes",
    "owsas",
    "agoldis",
    "AlexandreHetu",
    "dplewis",
    "yomybaby",
    "pocketcolin",
    "rdhelms",
    "jlnquere",
    "yagotome",
    "tybi",
    "RaschidJFR",
    "jeffgukang",
    "buitanloc",
    "LinusU",
    "REPTILEHAUS",
    "JeromeDeLeon",
    "kentrh",
    "L3K0V"
  ],
  "dangerLevel": "ScopedAndTested",
  "headCommitAbbrOid": "14f599c",
  "headCommitOid": "14f599c08e10a207c5e33598dfc4867fd96ceb1a",
  "mergeIsRequested": true,
  "stalenessInDays": 2,
  "lastCommitDate": "2020-06-27T20:03:56.000Z",
  "lastCommentDate": "2020-06-30T16:29:04.000Z",
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/45748/files",
  "hasMergeConflict": false,
  "authorIsOwner": false,
  "isFirstContribution": false,
  "popularityLevel": "Well-liked by everyone",
  "anyPackageIsNew": false,
  "packages": [
    "parse"
  ],
  "files": [
    {
      "filePath": "types/parse/v1/parse-tests.ts",
      "kind": "test",
      "package": "parse"
    }
  ],
  "hasDismissedReview": false,
  "ciResult": "pass",
  "lastReviewDate": "2020-06-30T12:30:57.000Z",
  "reviewersWithStaleReviews": [],
  "approvalFlags": 2,
  "isChangesRequested": false
}

🔔 @ullisenmedia @dpoetzsch @jaeggerr @flavionegrao @wesleygrimes @owsas @agoldis @AlexandreHetu @dplewis @yomybaby @pocketcolin @rdhelms @jlnquere @yagotome @tybi @RaschidJFR @JeffGuKang @buitanloc @LinusU @REPTILEHAUS @JeromeDeLeon @kentrh @L3K0V - 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.

Sorry, it would probably be helpful to provide the error that occurs when running tests.

Error in parse/v1
Error: C:/Repos/DefinitelyTyped/types/parse/v1/parse-tests.ts:175:5
ERROR: 175:5  expect  TypeScript@4.0 compile error:
Type 'Collection<Object>' is not assignable to type 'TestCollection'.
  The types of 'add(...).chain().map' are incompatible between these types.
    Type '{ <TResult>(iterator: never, context?: any): _._Chain<TResult, TResult[]>; <K extends "add" | "reset" | "get" | "model" | "models" | "query" | "comparator" | "initialize" | "at" | ... 12 more ... | "unbind">(iterator: K): _._Chain<...>; (iterator: string): _._Chain<...>; (iterator: Partial<...>): _._Chain<...>; }' is not assignable to type '{ <TResult>(iterator: never, context?: any): _._Chain<TResult, TResult[]>; <K extends "add" | "reset" | "get" | "model" | "models" | "query" | "comparator" | "initialize" | "at" | ... 12 more ... | "unbind">(iterator: K): _._Chain<...>; (iterator: string): _._Chain<...>; (iterator: Partial<...>): _._Chain<...>; }'. Two different types with this name exist, but they are unrelated.
      Types of parameters 'iterator' and 'iterator' are incompatible.
        Type 'Partial<Parse.Collection<Object>>' is not assignable to type 'Partial<Parse.Collection<Parse.Object>>'.
          Types of property 'add' are incompatible.
            Type '((models: any[], options?: Parse.Collection.AddOptions | undefined) => Parse.Collection<Object>) | undefined' is not assignable to type '((models: any[], options?: Parse.Collection.AddOptions | undefined) => Parse.Collection<Parse.Object>) | undefined'.
              Type '(models: any[], options?: Parse.Collection.AddOptions | undefined) => Parse.Collection<Object>' is not assignable to type '(models: any[], options?: Parse.Collection.AddOptions | undefined) => Parse.Collection<Parse.Object>'.
                Type 'Parse.Collection<Object>' is not assignable to type 'Parse.Collection<Parse.Object>'.

    at C:\Repos\DefinitelyTyped\node_modules\dtslint\bin\index.js:198:19
    at Generator.next (<anonymous>)
    at fulfilled (C:\Repos\DefinitelyTyped\node_modules\dtslint\bin\index.js:6:58)

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

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

Hi @reubenrybnik thanks for the heads up. I'm currently testing your commit 8adf160 (the one just before the parse update) and I see no issues on running $ npm test. In any case I believe the packages are independent from each other so you shouldn't been concerned about packages other than that in which you're making changes.

So I think it's best that you revert changes made to the parse package. I'm not sure why the tests are failing on your end, does that happen when you get a fresh checkout of master? Meybe you need to clean and reinstall node_modules to make sure everything is up to date. Let me know how that goes.

Hello @RaschidJFR, thanks for taking a look at this! It looks to me like 8adf160 is the commit just after the parse test update commit 1955fc1 instead of the one before it, which I believe is afe1393, so that might be why you had trouble replicating this. To better demonstrate this issue, I reverted 1955fc1, opened #45763, waited for the CI check for the PR to fail with the error I mentioned, and converted it to a draft to keep it off the board for the moment, which is admittedly something I should have probably considered doing before opening this PR to provide more concrete context for the issue. Let me know if that addresses your concerns around this change seeming like it might not be necessary.

It's probably also worth noting that per common mistakes, "Using the types Function and Object is almost never a good idea. In 99% of cases it's possible to specify a more specific type." so I would recommend making this change even if it were not an obstacle to the CI check for #45763.

With regards to the packages being independent from each other, parse/v1/index.d.ts appears to reference Underscore, which is likely why the CI check is running and failing tests for parse/v1 in a PR that only changes Underscore.

@reubenrybnik I think maybe this feels like a particularly strange error on our end because that whole parse/v1 folder is more for reference at this point and is not even published with the @types/parse package anymore. (@RaschidJFR correct me if I'm wrong here, but according to the package.json I think we're only publishing index.d.ts and the ts3.7 folder). I don't think the v1/index.d.ts file has been touched by Parse maintainers since 2018, and the only commit on that file in 2019 was actually by another @types/underscore maintainer who probably ran into a similar dependency as what you're seeing.

So I guess my suggestion is to just go ahead and delete that whole v1 folder. I don't see what it's possibly being used for at this point, and it seems to be causing trouble.

Thanks for looking into this in such depth @rdhelms, I appreciate it! I'd be happy to delete that folder instead if that would be the preferred fix, but if it's ok with you I'd prefer to close this PR and open a new one with those changes instead of making those changes on this branch.

Too, while I'm happy to do so I don't need to be the one to open the new PR if one of the Parse contributors would prefer to make this change themselves.

I agree with @rdhelms , let's remove the folder. @reubenrybnik I think the easiest path is just deleting de folder in a new commit on this same PR. Maybe update the title, but I don't think it's necessary to create a new one. Just make the changes and we'll approve it so you can continue with your other PR in underscore.

Hmm, unfortunately @RaschidJFR and @rdhelms deleting the contents of this folder does not make npm test happy. The error I get is:

Error: Could not find version 1.*
    at TypingsVersions.getLatestMatch (C:\Repos\DefinitelyTyped\node_modules\@definitelytyped\definitions-parser\dist\packages.js:272:19)
    at TypingsVersions.get (C:\Repos\DefinitelyTyped\node_modules\@definitelytyped\definitions-parser\dist\packages.js:260:58)
    at AllPackages.tryResolve (C:\Repos\DefinitelyTyped\node_modules\@definitelytyped\definitions-parser\dist\packages.js:55:36)
    at changedPackageIds.map.id (C:\Repos\DefinitelyTyped\node_modules\@definitelytyped\definitions-parser\dist\get-affected-packages.js:7:62)
    at Array.map (<anonymous>)
    at Object.getAffectedPackages (C:\Repos\DefinitelyTyped\node_modules\@definitelytyped\definitions-parser\dist\get-affected-packages.js:7:40)
    at Object.getAffectedPackagesFromDiff (C:\Repos\DefinitelyTyped\node_modules\@definitelytyped\definitions-parser\dist\git.js:81:39)
    at process._tickCallback (internal/process/next_tick.js:68:7)

Regrettably this error doesn't provide context about what package it was looking into when it failed. Looking through the code in the stack trace I was able to piece together that it checks for dependent packages using /node_modules/@definitelytyped/definitions-parser/data/definitions.json and that that package lists parse-mockdb as taking a dependency on parse/v1 which I was able to verify in that definition's tsconfig.json, so to be able to make this change I'd first need to get rid of that dependency. The sole owner of those definitions appears to be @dpoetzsch, who is also one of the owners of this code, so I'm going to loop them into this conversation.

I was ok with doing a simple delete, but I'm worried that dealing with that dependency may require some significant work, and if so I would rather not be the one to do it. If there are any further simple actions you think it is reasonable for me to take here, let me know and I'll be happy to take them, but otherwise it would be nice to get this PR approved in its current state since it's a very small, seemingly reasonable, and most likely correct change to make that will more quickly unblock my ongoing efforts with Underscore's definitions.

Pfff what a mess... the interdependency looks like a growing issue as it implies extra work for one developer to fix external issues in unfamiliar packages. I think that's an issue for the DefinitelyTyped team to sort, I'm seeing that there are a few discussions going on.

So I guess removing folder v1 should be a task for maintainers of parse then because of its implications. Also, I'd like to say that making the changes that you did in parse won't hurt as temporary workaround, but now looking at the big picture there is now a responsibility on all of us of keeping backward compatibility, many people's code depend's on ours (like parse on underscore, like parse-mockdb on parse), so ideally any developer should make the updates in a way that they don't break external packages.

So to sum up: is there any other way that you go ahead without modifying parse, @reubenrybnik ? If it must be done, I guess we could give a hand approving your changes to parse (changing 'Object' to Parse.Object) if tests are passing, but I encourage all of us to keep changes as internal as possible.

Opinions @rdhelms ?

It does seem like it should ideally be possible to reference specific versions of types rather than needing to keep everything up-to-date with everything else @RaschidJFR, but I'm not sure if DT provides such a mechanism at this time. I can double-check, though.

If it's any consolation, this only affects test code and as I previously noted this change would probably be good to make even if #45763 didn't depend on it since Object should generally not be used.

Ha, I feel like a silly person, I went searching for how to do this for a while when I'm literally making changes right now to a forked previous version of definitions.

@RaschidJFR, I suppose a potential way to possibly deal with this that would still change parse but most likely only once more would be to create a folder in Underscore like v1.9, put the last set of type definitons for that version in that folder, and update parse/v1 to reference that older version of Underscore types that will likely never change. That being said, I'll need to look more into what exactly the limitations of versioned folders are (e.g. will it only allow major versions? because it looks like Underscore has only ever had one of those) or what version of definitions would actually be best to have parse/v1 reference (I can probably figure that out by looking at dependencies in package.json for the last released version of parse 1.x).

Too, I do still maintain that this would be an appropriate change to make even if it wasn't needed for anything else, so if this seems like a headache it could possibly be one that gets postponed until later (where later may possibly be never if you do end up removing parse/v1 altogether).

Regrettably documentation for maintaining old versions isn't 100% clear on the major-version-only question, but if I had to judge just from reading it I'd say that this strategy is only for use with major versions, which as I mentioned will not work well with Underscore since it's still on v1.

In the short term, I think it makes sense to just go ahead and merge this current PR @reubenrybnik - you're right that the change you're making is "better than it was," and in light of our entanglement with parse-mockdb that seems like the fastest way to let you move on with your work. I'm going to go ahead and approve this, but @RaschidJFR if you think the current change is problematic then we can discuss alternatives.

In the medium term, I think we should coordinate with @dpoetzsch to remove parse/v1 as a dependency from @types/parse-mockdb and then remove that folder on our end. It's true that in general we should try to minimize breaking changes, but I think the primary goal in a @types package should be to have the types be aligned with whatever the current version of the package itself is. In the case of parse-mockdb, the package has a dependency of "parse": "^2.0.0", and so I don't think there's any reason why its types should be specifically targeting the v1 types. In general I would say that even if we DO discover a package that is explicitly depending on a 1.x.x version of the parse package, it seems better to try to help get that package up to date rather than to keep older versions of types around in DefinitelyTyped.

@reubenrybnik Everything looks good here. Great job! I am ready to merge this PR on your behalf.

If you'd like that to happen, please post a comment saying:

Ready to merge

and I'll merge this PR almost instantly. Thanks for helping out! ❤️

Sounds good, thank you very much for your time @rdhelms and @RaschidJFR! Sorry this didn't end up being the most optimal solution from the perspective of parse, but I appreciate you allowing me to make this change to unblock myself sooner rather than later 😅.

@RaschidJFR, since it looks like you 👍'd the approval from @rdhelms I'm going to merge this an hour from now unless you ask me not to.

Ready to merge