nunjucks: fix ILoader.getSource

[not-my-profile]

Attempting to implement an async ILoader previously failed with the following type error:

Type '(name: string, callback: Callback<Error, LoaderSource>) => void'
is not assignable to
type '{ (name: string): LoaderSource; (name: string, callback: Callback<Error, LoaderSource>): void; }'.
Target signature provides too few arguments. Expected 2 or more, but got 1.

[typescript-bot]

@not-my-profile Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — I'm the local bot who will help you through the process of getting things through.

This is a live comment which I will keep updated.

1 package in this PR

• nunjucks — on npm, on unpkg

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.

You can test the changes of this PR in the Playground.

Status

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

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.

---

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 65883,
  "author": "not-my-profile",
  "headCommitOid": "6853cc2663bb306735ec1ef46ca2cdfb145a64d2",
  "mergeBaseOid": "d952cf91cf1bf44f412df927cf48a18670c0ca5d",
  "lastPushDate": "2023-06-28T14:12:12.000Z",
  "lastActivityDate": "2023-06-29T22:00:38.000Z",
  "mergeOfferDate": "2023-06-29T21:08:18.000Z",
  "mergeRequestDate": "2023-06-29T22:00:38.000Z",
  "mergeRequestUser": "not-my-profile",
  "hasMergeConflict": false,
  "isFirstContribution": true,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Popular",
  "pkgInfo": [
    {
      "name": "nunjucks",
      "kind": "edit",
      "files": [
        {
          "path": "types/nunjucks/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/nunjucks/nunjucks-tests.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "MatthewBurstein"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Popular"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "peterblazejewicz",
      "date": "2023-06-29T21:07:33.000Z",
      "isMaintainer": true
    },
    {
      "type": "approved",
      "reviewer": "rslabbert",
      "date": "2023-06-29T00:11:40.000Z",
      "isMaintainer": false
    }
  ],
  "mainBotCommentID": 1608832505,
  "ciResult": "pass"
}

[typescript-bot]

🔔 @MatthewBurstein — 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.

[DangerBotOSS]

Inspecting the JavaScript source for this package found some properties that are not in the .d.ts files.
The check for missing properties isn't always right, so take this list as advice, not a requirement.

nunjucks (unpkg)

was missing the following properties:

1. compiler
2. parser
3. lexer
4. nodes
5. reset

Generated by 🚫 dangerJS against 6853cc2

[not-my-profile]

CC @rslabbert, since you suggested the overloading.

[rslabbert]

@not-my-profile while I think this approach makes sense, what do you think of the following? I believe it increases the typescript version requirement to ensure conditional types are available and might come with some strictness requirements, but it seems to model what is needed.

I'd be happy to approve the PR as is, but I think either way it's probably a breaking change?

interface ILoader<Async extends boolean | undefined> {
  async: Async;
  getSource: Async extends true
    ? (name: string, callback: Callback<Error, LoaderSource>) => void
    : (name: string) => LoaderSource;
}

export interface WebLoaderOptions {
  useCache?: boolean;
  async?: boolean | undefined;
}

export class WebLoader<
    Opts extends WebLoaderOptions,
    Async extends Opts["async"] extends true ? true : false
  >
  extends Loader
  implements ILoader<Async>
{
  constructor(baseUrl?: string, opts?: Opts);
  async: Async;
  getSource: Async extends true
    ? (name: string, callback: Callback<Error, LoaderSource>) => void
    : (name: string) => LoaderSource;
}

const _async = new WebLoader("", { async: true });
_async.getSource("", () => {});

const sync1 = new WebLoader("", { async: false });
sync1.getSource("");

const sync2 = new WebLoader("");
sync2.getSource("");

[not-my-profile]

Ah yeah ... conditional types are an interesting idea but that doesn't work because the async property can also be missing from the interface, and if you change it to async? then the inference no longer works, e.g. the following type checks:

interface Foo<A extends boolean> {
    foo?: A extends true ? true : false | undefined;
}

const A: Foo<true> = {};

But I think I found a nice solution by defining:

type ILoader = ILoaderSync | ILoaderAsync | WebLoader;

see the updated PR.

I'd be happy to approve the PR as is, but I think either way it's probably a breaking change?

Yeah but as Ryan put it:

If you change a type, any type, anywhere, in any fashion, that change is always capable of breaking downstream code.

(and npm limits us to three numbers in the version ... so I think a patch version bump is the best we can do for this)

[rslabbert]

That makes a lot of sense, I feel like there is an elegant solution but without doing a major restructure (difficult given the underlying code) I think this is a good place to be.

This change would definitely have to be a major bump though because changing an interface to a type like this will make it no longer work in an implements ... spot which would be a pretty common situation in which to use it.

Also I just realised I'm no longer actually marked as a reviewer on this due to a GitHub username change, so my approval is in spirit only.

[not-my-profile]

I feel like there is an elegant solution

Nunjucks should just use Promises but I don't think there's anything better we can do here.

This change would definitely have to be a major bump though

Yes according to SemVer but the @types packages do not appear to follow SemVer ... the assumption is that MAJOR.MINOR always correspond to the package that is described by the type definitions (see also #34047). You literally declare:

// Type definitions for nunjucks 3.2

Changing that comment to nunjucks 4.0 would be wrong ... there is no "nunjucks 4.0". And if the major version of a type definition were to converge from the major version of the described package, we'd need to somehow document that version mapping and there currently does not exist any tooling support for that.

But as you pointed out breaking SemVer so prominently still sucks, so I changed the union type definition to:

export type ILoaderAny = ILoader | ILoaderAsync | WebLoader;

to avoid breaking implements ILoader.

Also I just realised I'm no longer actually marked as a reviewer on this due to a GitHub username change, so my approval is in spirit only.

I am not that familiar with the PR automation here but the lifecycle diagram does say "reviews from contributors" so maybe your review could greenlight this PR even if you are no longer registered as a code owner.

[not-my-profile]

/cc @peterblazejewicz this PR has been approved by Ruben who was a type definition owner but isn't anymore because of a username change, and the only remaining owner doesn't appear to be active anymore, so it'd be great if you could review this or move it to the "Needs Maintainer Action" board, so that this doesn't unnecessarily lie idle for two weeks.

[peterblazejewicz]

@not-my-profile thanks!
LGTM!

@rslabbert please consider adding back your handle, if you don't mind!

[typescript-bot]

@not-my-profile: Everything looks good here. I am ready to merge this PR (at 6853cc2) on your behalf whenever you think it's ready.

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! ❤️

(@MatthewBurstein: you can do this too.)

[not-my-profile]

Ready to merge