Duplicate .guard.ts files

[boompikachu]

When calling ts-auto-guard, all of the index.ts file that exported the type with /** @see {...} ts-auto-guard:type-guard */ has their own index.guard.ts

Similar issue to https://github.com/rhys-vdw/ts-auto-guard/issues/169 but --export-all flag is not used here

[rhys-vdw]

Hm. That's not good at all. Surprised this hasn't come up before.

[rhys-vdw]

Just a heads us: I'm not actively working on this project right now, but I will review any community contributions and push updates. If you want to have a go at fixing this one @boompikachu (or anyone else) your contribution would be appreciated.

I feel like this was definitely not a problem at one stage. The issue seems to be here:

https://github.com/rhys-vdw/ts-auto-guard/blob/f653b342181a54434bfdf774c22314a9205f2eab/src/index.ts#L326-L329

Honestly this may actually be best handled by changing the API for ts-auto-guard from:

/** @see {isPerson} ts-auto-guard:type-guard */
interface Person { }

to

// ts-auto-guard isPerson
interface Person { }

Which was actually my original plan, but I couldn't work out how to get the leading comments when I first wrote this library (I feel like this didn't exist at the time).

My thinking is that if we use the comments on the actual exported symbol, rather than doc comments that are attached to the type, we can be very specific about where the guards are generated. For example if you wanted to only generate index.guard.ts:

// color.ts
export type Color = "red" | "blue";

// index.ts
import { Color } from "./color.ts";

// ts-auto-guard isColor
export Color;

Interested if anyone has thoughts on the above.

[download13]

I have a possible solution for this, but it would have knock-on effects.

It seems like a decision needs to be made about how we handle generating guards for external types. Should guards be generated for types that did not originate in a file if the file re-exports them?

Right now the library seems to do that, and judging by #191, some people are using this functionality.

It definitely doesn't make sense to regenerate a whole type guard when one already exists, so the guard for a file that re-exports types should re-export the existing guards instead of generating a new copy. In order to do this consistently, we would also need to ensure that guards are generated at the leaves of the dependency tree first.

Currently the library appears to attempt this, but doesn't always succeed. I extended the test case given above, by adding a zustom.ts file that exports another type which is re-exported by index, and the guard is built in both index.guard.ts and zustom.guard.ts, but zustom also imports index's guard instead of the other way around. I assume this is because it is processed last, because it's name starts with a z.

Removing the re-export guard functionality would simplify how this library works since we wouldn't have to worry about dependency order (or what to do in the event of circular dependencies).

If we do away with guards for re-exported types, then we can filter the exports array at src/index.ts:1064 to remove all exports that did not originate in the current file. I've tested it and this does solve the current problem, but might break some people's use-cases.

There is a sort of middle-ground where we could allow generation of guards for re-exported types if the re-exported type came from a file in node_modules, or simply turn the capability on with a flag and people would need to invoke the CLI separately for each mode of operation.

[rhys-vdw]

Hey @download13, was away for the weekend.

It seems like a decision needs to be made about how we handle generating guards for external types. Should guards be generated for types that did not originate in a file if the file re-exports them?

My opinion on this, as I believe I explained above, is that the export should happen in the file that contains the export instruction comment.

This way you will only get a duplicate generation if one guard is marked twice, leaving it entirely a decision (or mistake?) of the user.

This feels like an elegant solution to me. Do you see any problems with it?

It definitely doesn't make sense to regenerate a whole type guard when one already exists, so the guard for a file that re-exports types should re-export the existing guards instead of generating a new copy. In order to do this consistently, we would also need to ensure that guards are generated at the leaves of the dependency tree first.

Currently the library appears to attempt this, but doesn't always succeed.

Yes, this is true and its' a separate issue. It is meant to recycle guards, the intention there was to reuse the guards as an implementation of a different type guard.

e.g.

type Person = { name: string }
type Persons = Person[]

function isPerson(obj: any) bool is Person {
  return obj != null && typeof obj.name === "string";
}

function isPersons(obj: any) bool is Persons {
  return Array.is(obj) && obj.map(isPerson);
}

I don't think it's creating a dependency tree of the different types, and is thus failing to create the guards in the correct order. I would say this is a separate issue. I'm also open to removing that feature entirely in the interest of simplifying the code base (although I agree it's nice).

[download13]

My opinion on this, as I believe I explained above, is that the export should happen in the file that contains the export instruction comment.

Ah, apologies, I didn't understand the implications of the comments when I read your post.

I believe the same technique I mentioned for the declarations will work on the jsdoc tags, but this brings up another complication.

Do you have any thoughts on what to do when --export-all is being used? In that case we can't rely on the jsdoc tags to differentiate which ones are supposed to have guards generated.

My only thought so far is that we could have exportAll cause the generator to behave as though declarations in their original file have the jsdoc on them. Re-exports would not get the pretend jsdoc tag, but could still have a real one.

I think that would fix this issue, and allow generating guards for re-exported types to continue to be maintained as a feature.

This would definitely be a breaking change as it would alter the behavior of --export-all.

[rhys-vdw]

I didn't understand the implications of the comments when I read your post.

All good!

Do you have any thoughts on what to do when --export-all is being used? In that case we can't rely on the jsdoc tags to differentiate which ones are supposed to have guards generated.

I think if you're using --export-all you should be whitelisting the files that you want to export. Generally speaking I don't really like --export-all, and I'm happy for its behaviour to remain quite naive and essentially be a second class citizen in terms of features.

[rhys-vdw]

I believe the same technique I mentioned for the declarations will work on the jsdoc tags, but this brings up another complication.

My thinking here was that JSDoc tags get associated with the type, where regular comments are associated with the code itself. However I'm not sure if this is true.

In truth I don't really like the JSDoc tags, the only reason they're there is because the original version of ts-morph I was using didn't have support for regular comments (or I couldn't find it). I might be reaching for a reason why they're bad haha. If it's not the case then we can ignore that point.

[download13]

Nodes in ts-morph keep a reference to the source file they were originally declared in, and by comparing it to the file we are currently processing we can always check if a declaration is at home or a re-export. This seems to be true for types and JSDoc tags.

I think if you're using --export-all you should be whitelisting the files that you want to export.

Does passing them to the CLI count as whitelisting, or do you mean in some other way?

What I was attempting to do was keep the behavior as close as possible to the way it works currently, with one omission that would solve the current issue.

--export-all would generate guards for exported types that are originally declared in a file passed to the CLI, but not for re-exported types.

So for example, a file:

export type Color = 'red' | 'blue' | 'green'

passed to ts-auto-guard --export-all would generate:

/*
 * Generated type guards for "custom.ts".
 * WARNING: Do not manually change this file.
 */
import { Color } from "./custom";

export function isColor(obj: unknown): obj is Color {
    const typedObj = obj as Color
    return (
        (typedObj === "red" ||
            typedObj === "blue" ||
            typedObj === "green")
    )
}

however, the following file, in the same directory:

export { Color } from './custom'

would generate nothing.

To make the generator create a guard for a re-exported type you'd need to do as you mentioned in 191, and deliberately create a new type alias:

import { Color as RenamedColor } from './custom'

/** @see {isColor} ts-auto-guard:type-guard */
export type Color = RenamedColor

[rhys-vdw]

Nodes in ts-morph keep a reference to the source file they were originally declared in, and by comparing it to the file we are currently processing we can always check if a declaration is at home or a re-export.

Ah, nice. Yeah that's a good fix!

Does passing them to the CLI count as whitelisting, or do you mean in some other way?

I did mean by passing them to the CLI, yes. Should have been clearer.

--export-all would generate guards for exported types that are originally declared in a file passed to the CLI, but not for re-exported types.

Ah yes, that sounds really sensible!

In summary, the desired change is:

Check the jsdoc tag to ensure that it was indeed included in the current file. This will naturally give the desired behaviour to --export-all.

Very elegant!

[rhys-vdw]

Further thoughts:

Might cause some issues for reopened types with --export-all, but that's its own issue (#29).

Perhaps this won't be so easy with --export-all because there is no JSDoc node. We'll instead need to check for the original declaration file. If this is an issue I'd be happy to fall back on the user having to whitelist the files they want. The most important thing is that the JSDoc annotated generation is working as expected.