Ensure public types don't reference internal types

[benmccann]

This helps with #4011. Right now there are a lot of references to types that aren't documented which makes things hard to follow in some cases

If we wanted to enforce this we could (#4020), but I'm not sure that we'll need to

[changeset-bot]

⚠️ No Changeset found

Latest commit: 8c35bf7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[dummdidumm]

This makes sense to me. It always bugged me (I'm Svelte core, too) that some types that are used by public types were not exported or marked as internal, which doesn't make sense to me since everything that a public type uses is also public.

[Rich-Harris]

I think there are some things that should probably remain internal. For me the threshold isn't 'is this a transitive dependency of a public type?', it's 'would I ever need to import it?'. For example utilities like MaybePromise and Either are really just implementation details. Similarly, RequiredResolveOptions is a dependency of the public ResolveOptions because of this...

export type ResolveOptions = Partial<RequiredResolveOptions>;

...but at no point would you need to use it. (We could maybe flip it around and have Required<ResolveOptions> instead, but that's an aside.)

Of course, there is a case for documenting some of the internal types, even if we don't make them public. For example it's good to know what's available on Logger, even though you'll never need to create one yourself. I wonder if we should have a category of those types that can be included in the docs but which aren't otherwise public.

It's worth being conservative about this — once something is a public type we can't amend or remove it without a major version bump.

[Rich-Harris]

when I suggested documenting internal types without making them public, I didn't mean putting them in index.d.ts but not exporting them. i think they should still live in a separate module, perhaps utils.d.ts or something

[Rich-Harris]

since this is used in a bunch of places perhaps it would make sense to have a RequiredResolveOptions type in internal.d.ts

[benmccann]

I'd thought about it, but it only saves two characters and adds a layer of indirection, so is it even worth it?

[benmccann]

I got rid of the RequiredResolveOptions and removed the export from the new types added

when I suggested documenting internal types without making them public, I didn't mean putting them in index.d.ts but not exporting them. i think they should still live in a separate module, perhaps utils.d.ts or something

What's the difference between the two approaches? They seem equivalent to me. Putting it in index.d.ts makes it a little clearer I think that you can't rename / remove methods without breaking our users, which people might be more inclined to accidentally do in a utils.d.ts

[Rich-Harris]

You can rename and remove them, that's the whole point!

[benmccann]

I'm rather confused about what you're suggesting. Are you saying that adapters should not call Builder.log or that they can but that you want to be able to rename the methods on it requiring new versions of the adapters without considering it a breaking change?

It seems to me like we'd just be covering our eyes and ears to the fact that we're making breaking changes. Things like Logger already are public. We can't remove or rename the methods on it without breaking our users.

[Rich-Harris]

Ah, I see what you mean now. I thought you were saying that you couldn't rename it to LoggyLogLogFriend or change MaybePromise to GoodIntentions without breaking stuff, which you can.

My main concern is that we need to differentiate public types from semi-public types in the docs — at the moment the only difference between Adapter and AdapterEntry is the presence or absence of the export keyword, and the docs specify that both can be imported which isn't correct. They should probably be in separate sections entirely:

(Arguably AdapterEntry and RouteDefinition are two examples of types that should be public, though the names could probably do with some bikeshedding.)

[benmccann]

I took a brief look at separating the exported and non-exported types and it's not super obvious to me how I'd do it. I updated the wording so that it's accurate now. Feel free to take this over if you'd still like to separate them. I don't have much of a preference for whether it's done by omitting the export keyword or moving them to another file.

[benmccann]

Closing in favor or #4104