Type errors declaring a new language definition in TypeScript

[jsharkey13]

Describe the issue/behavior that seems buggy

I am attempting to declare and register a new language definition for an internal pseudolanguage (aimed at students, and very simplistic), but doing so in TypeScript and not plain JS. I am using 11.1.0.
I declare the new rules in a function I want to pass into hljs.registerLanguage(...) using the LanguageFn type you export from index.d.ts. However LanguageFn annotates the hljs argument as optional, which means that I cannot use something like hljs.QUOTE_STRING_MODE in my function without a guard against hljs being undefined as the type annotation says it might be.

Sample Code or Instructions to Reproduce

A minimal TypeScript example would be:

import {LanguageFn} from "highlight.js";

const myLanguageRules: LanguageFn = function(hljs) {
     return {
        name: 'myLanguage',
        contains: [
            hljs.QUOTE_STRING_MODE,
            hljs.NUMBER_MODE,
        ]
    };
}

where the references to hljs.QUOTE_STRING_MODE and number mode will cause type errors since hljs has the type HLJSApi | undefined (the error on trying to use hljs is TS2532: Object is possibly 'undefined'.). This is such a simple case that you can see it will cause a problem without even needing to use the compiler.

The only way to do this guard in a way that allows me to use hljs in many places in my definition seems to be the incredibly hacky if (!hljs) return {} as Language; at the start, otherwise the type system complains about the return type of the function being incorrect. However I don't understand why hljs is marked as optional in the definition export type LanguageFn = (hljs?: HLJSApi) => Language in the first place? Surely it is always passed in by the library?

Expected behavior

I would expect the LanguageFn type to actually be export type LanguageFn = (hljs: HLJSApi) => Language without the argument being optional, and then if I choose not to use the hljs argument I can just name it _hljs in my own function and TypeScript knows this means I do not intend to use the argument.

I could foresee changing this signature causing issues for anyone using TypeScript to define languages of their own if (and probably only if) they do not have a hljs argument at all in their function. Otherwise it seems safe to change. Was there a reason it is declared the way it is? Is it autogenerated, or written by hand?

[jsharkey13]

I don't know why it added the parser label by itself, but it's a declared types question.

I have a further question about the ModeDetails type declared in the same types definition file too; would that be best as another issue so it can be addressed separately?

[joshgoebel]

Let's keep them separate.

[joshgoebel]

I don't know why it added the parser label by itself, but it's a declared types question.

parser is kind of a catch-all, and merely one of the default tags, but as a catch-all it does apply here me thinks.

I don't understand why hljs is marked as optional in the definition export type LanguageFn = (hljs?: HLJSApi) => Language in the first place?

I think this is a result of how I was thinking about types/interfaces as requirements, as in how they force your code to conform to the type/interface. In this case accepting a hljs instance is not a hard requirement - a grammar can work just fine without this provided doesn't need any of the API - so therefore it was marked optional so as not to force it on anyone.

Surely it is always passed in by the library?

Yep.

---

only way to do this ... seems to be the incredibly hacky ...

I'm going to get one other person's thoughts on this but at first glance this is quite annoying indeed and it does seem we should change this to specify that the parameter is required rather than optional.

[SleeplessByte]

I think this is a result of how I was thinking about types/interfaces as requirements, as in how they force your code to conform to the type/interface. In this case accepting a hljs instance is not a hard requirement - a grammar can work just fine without this provided doesn't need any of the API - so therefore it was marked optional so as not to force it on anyone.

That's actually interestingly not the case in TypeScript!

type MyFunction = () => {}
type MyOptionalFunction = (arg?: string) => {}
type MyArgFunction = (arg: string) => {}

Consider these three function types/signatures.

See this playground

// This is fine
const test1: MyFunction = () => {}

// This is fine
const test2: MyOptionalFunction = () => {}

// This is fine
const test3: MyArgFunction = () => {}

// This is fine because the arguments are optional, so it can
// be undefined. () is the same as (arg: string | undefined) 
// and always passing in undefined.
const test4: MyFunction = (arg?: string) => {}

// This is not fine, the argument MUST be optional, otherwise
// this will break when it's not given or when undefined is
// passed in.
//
// @ts-expect-error
const test5: MyOptionalFunction = (arg: string) => {}

// This is fine because arg?: string is WIDER than arg: string
const test6: MyArgFunction = (arg?: string) => {}

// This is fine because string | number is WIDER than string
const test7: MyArgFunction = (arg: string | number) => {}

// Fine because it expects no args
test1()

// Not fine because it expects no args
//
// @ts-expect-error
test1('fine')

// Fine because it's optional
test2()

// Fine because it's allowed, despited test2 not using it
test2('fine')

// Not fine, despite test3 not using it, because it's marked
// as required.
//
// @ts-expect-error
test3()

// Fine
test3('fine')

// Fine 
test4()

// Not fine, despite test4 allowing it in its anonymous
// definition. MyFunction doesn't allow it, therefore it's
// not allowed
//
// @ts-expect-error
test4('fine')

So when you write:

type LanguageFn = (hljs?: HLJSApi) => Language

It means:

• LanguageFn is a function that returns Language (or throws)
• LanguageFn may be called with an argument. It may be undefined and it may be of shape HLJSApi. It's not required to consume it. Calling it with an argument is optional.

In the same way when you write:

type LanguageFn2 = (hljs: HLJSApi) => Language

It means:

• LanguageFn2 is a function that returns Language (or throws)
• LanguageFn2 is always called with one argument. That argument is always HLJSApi in shape. It's not required to consume it.

@jsharkey13 was right that because it's always called with the argument, the argument shouldn't be optional. However, James, for future reference, if you require the argument to produce a sane response, the right way to handle that is:

const myLanguageRules: LanguageFn = function(hljs) {
  if (!hljs) {
    throw new Error('HLJSApi is required for this language')
  }
  
  // Here your type is narrowed to HLJSApi without undefined
}

It's not hacky to require to narrow the type if the argument is truly optional.

[joshgoebel]

That's actually interestingly not the case in TypeScript!

Ah, but of course - and thanks for detailed examples. I think I was maybe confusing arguments with keys on object literals (where TS forces your hand much more). That's just a guess on why I went this way originally.

So it seems the PR in question should resolve this then.

[jsharkey13]

@SleeplessByte - ah, thank you! (And neat example cases too!)
I'd not put much thought into it, but yes, throwing an error is clearly the right thing to do. I'm not sure what highlight.js would do if the registration of the language encountered an error, but I guess my own code could just wrap the registration in a try-catch if I was actually worried about it propagating.

I agree the PR will fix it. test3 above even suggests that the change will not break the case where someone declared their own function without the hljs argument which is the only case I'd worried about.

[joshgoebel]

I'm not sure what highlight.js would do if the registration of the language encountered an error,

Fall back to substituting a plaintext grammar and log an error in production mode. (or blow up in debug mode)

even suggests that the change will not break the case where someone declared their own function without the hljs argument which is the only case I'd worried about.

Yes, quite convenient. :)