fix(type): add disabled to pino.LoggerOptions

[JonParton]

fixes #1628

[mcollina]

Could you add a test for this with tsd?

[JonParton]

Hi @mcollina - Sorry it has taken me an absolute age to circle back around on this.

So interestingly, there is actually a tsd test for this already but it was happy even before I added this type as it seems to be fine with extra keys on passed type objects, just not wrongly typed ones ...

pino/test/types/pino.test-d.ts

Lines 88 to 108 in 062e30a

	pino({
	browser: {
	write: {
	info(o) {},
	error(o) {},
	},
	serialize: true,
	asObject: true,
	transmit: {
	level: "fatal",
	send: (level, logEvent) => {
	level;
	logEvent.bindings;
	logEvent.level;
	logEvent.ts;
	logEvent.messages;
	},
	},
	disabled: false
	},
	});

So in the case of this test, disabled: false was getting no complaints as there were no types for that LoggerOptions key.

However, now that I've added a type for the disabled key, if you change that pino instantiation to have a key of disabled: 2 pnpm test-types will now give an error.

This seems to be an issue with typescript using structural typing instead of nominal typing, extra keys are generally allowed when passing a type and so no error!

A way around this seems to be to change the tests to not be instantiating pino instances but instead assigning to a variable of type LoggerOptions

const loggerOptions1: LoggerOptions = {
    browser: {
        write: {
            info(o) {},
            error(o) {},
        },
        serialize: true,
        asObject: true,
        transmit: {
            level: "fatal",
            send: (level, logEvent) => {
                level;
                logEvent.bindings;
                logEvent.level;
                logEvent.ts;
                logEvent.messages;
            },
        },
        disabled: false,
        rubbish: 1, // Will error with a usually horrible looking typescript message!
    },
};

However, this is being done all over the place and ties it to the specific defined types rather than use of the pino instantiation ... So over to you for if you want to make a change that big to the type checks! 😵‍💫

ℹ️ kudo's to this SO Question also giving me a pointer on this!

[mcollina]

I'm sorry but I don't understand the problem, as all tests are currently passing. We need a new test for this change, and the current tests should keep passing to land this change.

[JonParton]

but @mcollina there was already a test where the disabled property was being used as I linked above which is what I was expecting to have to add to answer your request...

I don't see any other tests anywhere that would check that a type for a property already exists for any other properties so this would be a change of approach in the project testing ...

Do you want me to make a specific test for this one property even though none of the others are checked in this way? Or did I miss tests some where else? Sorry if I have! 🤦

[JonParton]

The specific line that is testing the type of this property already! (But against no defined type without this PR)

pino/test/types/pino.test-d.ts

Line 106 in 062e30a

disabled: false

[mcollina]

I don't understand the digression on adding tests for custom keys, it seems irrelevant to the issue at hand. Replying with "there is already a test at ..." would have been a good answer.

What I do not understand is why the tests did not catch that property being missed. It's possible that all the above it's related to those, could you confirm? If that's the case, a follow up PR would be amazing, but this can land as is.

[JonParton]

What I do not understand is why the tests did not catch that property being missed. It's possible that all the above it's related to those, could you confirm?

All the above is indeed related to that!

Basically I saw there was a test already for this property and went to find out why it wasn't failing ... That caused all of the afore mentioned digression!..

Agreed it would be a separate PR to fix this hole in testing and see if any other properties are missed in the type defs!

Happy for this to be merged if you are! 👍

[mcollina]

lgtm

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.