Make it harder to accidentally use internal APIs

[calebmshafer]

trafficstars

With our current release tag setup we use the @internal tag to denote something that we do not want consumers of the library to use. However, those APIs are still part of the barrel files and exported from our packages, either as full types or a method/property on a public type. This leads to consumers accidentally using these APIs because they do not know any difference.

We need to come up with a strategy that reduces, or optimally eliminates, the ability for consumers to run into this scenario.

[pmconne]

I scheduled a pow-wow.

[calebmshafer]

As a result of the aforementioned "pow-wow", we are going to take this in steps to validate what a good strategy will be. Below is the summary of the action items of the call.

• [ ] Move as many internal APIs our of the main barrel exports
  • The plan to move them out will be to create a separate folder in each package as a peer of src, named src-internal to make it explicit
  • We need to understand the scope of this change to determine whether we can do it all, or at least, a subset of it within 4.x or it needs to wait until 5.0.
  • [ ] @pmconne is going to get the list of what APIs can be moved out of core-common as a template. Then we break out from there so that more people can tackle each and gather the data
  • [ ] Scope of all packages (after @pmconne's work)
• [ ] Investigate strip internals (@wgoehrig)
  • This will require 5.0 release
  • Another option is to use the api-extractor feature set but that seems like more custom tooling so we wanted to start with the Typescript feature. https://api-extractor.com/pages/setup/configure_rollup/
• Do not try and tackle alpha/beta for now, focus on internal

[calebmshafer]

@GerardasB @grigasp @saskliutas FYI - I know the AppUI and Presentation side is looking at some similar so wanted to share what was discussed.

Presentation pkg related issue - https://github.com/iTwin/presentation/issues/545

[pmconne]

@calebmshafer @aruniverse @wgoehrig @ben-polinsky One perennial pain point with our API support policies is that we can never add new required properties to a @public interface, just in case somebody outside itwinjs-core is creating their own implementation of it. On the other hand, exposing interfaces instead of implementations (classes) gives us more flexibility to make improvements to the implementation without breaking the API. We really have two kinds of interfaces:

• Those that define the shapes of objects provided by itwinjs-core APIs. Consumers of the library are expected to obtain implementations of these objects from itwinjs-core, not define their own implementations. Maybe call these "output interfaces".
• Those that define the shapes of objects that are passed in to itwinjs-core APIs. Consumers are expected to define their own implementations.

If we had a way to enforce that a particular interface is an output interface and prohibit anyone from defining their own implementation, we would be free to add new properties to it without any risk of breaking consumers. I made various unsuccessful attempts, but now I think I figured out something that will work.

Here's a simple example.

Here's a real example in core-backend: SettingsSchema is the non-instantiable interface; SettingsSchemaImpl is the internal implementation thereof.

I see a couple of small wrinkles that don't bother me much:

• Methods need to be declared as function properties. I don't think our docs generator currently produces good documentation for those. I assume that can be fixed.
• We will need to explain this "class masquerading as interface" concept to people.

Do you see any issues I overlooked? Do you have strong objections to this approach? It will make promoting APIs to public much less risky.

EDIT: Now I belatedly realize the private constructor doesn't actually prevent people from supplying their own implementations.

[pmconne]

What do you think about relying on the @internal release tag and people obeying very clear instructions?

export interface OutputInterface {
  /** @internal */
  readonly doNotInstantiateThisInterface: unknown;
}

export interface SettingsSchemas extends OutputInterface { /* ... */ }

class SettingsSchemasImpl implements SettingsSchemas {
  public readonly doNotInstantiateThisInterface = undefined;
  // ...
}

[pmconne]

A symbol would work, especially if the interface and implementation are defined in the same source file. Otherwise, you have to export the symbol from one source file. But you would not export the symbol from the package's barrel file, and stripInternal would make it difficult for consumers of itwinjs-core to find it.

[grigasp]

In our new packages we're trying out an approach to not export such types, e.g.:

/** not exported at all */
interface MyProps {
  someProp: number
}
/** not exported at all */
interface MyReturnType {
  someValue: string;
}
/** @public */
export function myFunction(props: MyProps): MyReturnType {
  return { someValue: "x" };
}

In typescript, consumers don't need to specify variable types, so in majority of cases this simply works:

const result = myFunction({ someProp: 123 });

In those occasional cases when they do need the type, they can do this:

type MyFunctionReturnType = ReturnType<typeof myFunction>;

One problem with this approach is that it becomes much harder to notice breaking changes:

• these interfaces don't get included into API summary files,
• they don't have release tags and aren't even exported.

We have a work item to find a solution to this.

[grigasp]

BTW, I think the problem of not being able to modify props-only or return-only interfaces in a non-breaking way is separate from the exported @internal APIs problem.

Regarding the latter, in our new packages we made sure we don't export any top-level @internal APIs through the barrel. However, we still have the problem with exported public interfaces exposing internal attributes.

[pmconne]

In those occasional cases when they do need the type, they can do this

If they do this and you later add a new required property to MyReturnType, you will break their code. I think that whether or not you export or document your types, if they appear in the signature of a @public function they are implicitly part of the public API and subject to the API support policy.

[grigasp]

If they do this and you later add a new required property to MyReturnType, you will break their code.

Not unless they're creating instances of MyFunctionReturnType or implementing it. But in that case, I'd argue it's not our problem, since our function returns an object that doesn't have a public type, so we should be free to add any members we want (but, of course, not remove them).

I think we shouldn't loose our sanity with overly strict rules, otherwise we may start considering cases like below, where we can't add a new export to a package without considering that a breaking change:

// adding an export to `core-bentley` package is a breaking change to this code:
import * as coreBentley from "@itwin/core-bentley";
type CoreBentleyApi = typeof coreBentley;
const value: CoreBentleyApi = {
  // define all core-bentley exports here
};

[pmconne]

our function returns an object that doesn't have a public type

How is anybody supposed to use your API if they can't know anything about its return type? Can you link to examples in your repo?

[grigasp]

our function returns an object that doesn't have a public type

How is anybody supposed to use your API if they can't know anything about its return type? Can you link to examples in your repo?

Public function: https://github.com/iTwin/presentation/blob/a7a331fecb20f1ec2b9441e3015e48c573a6b5bb/packages/hierarchies-react/src/presentation-hierarchies-react/UseTree.ts#L77

Usage: https://github.com/iTwin/presentation/blob/a7a331fecb20f1ec2b9441e3015e48c573a6b5bb/apps/test-app/frontend/src/components/tree-widget/StatelessTree.tsx#L69

In code these types act as if they were exported:

[ben-polinsky]

In our new packages we're trying out an approach to not export such types.

I was thinking about this as well. I like this approach best.

[pmconne]

I like this approach best.

I don't. I'd prefer if people can't accidentally discover that we have broken their code. I'd also prefer that types used in public function signatures are documented on the docs site.

I plan to use the Symbol approach unless somebody sees some fatal problem with it.

[ben-polinsky]

We could probably come up with a way to document them, but fair enough. Don't see any fatal flaws with the Symbol approach.

[grigasp]

I plan to use the Symbol approach unless somebody sees some fatal problem with it.

Not sure if this is a problem in itwinjs-core, where all packages are tightly coupled and released in a lockstep, but we found that exposing things like symbols and classes through public API kind of makes it a requirement to use the same version of such package across the whole app and its dependencies.

The same symbol/class from two different versions of the same package is always considered different by TS system:

// package deps:
A
- B
  - [email protected]
- [email protected]

// package C:
export interface X {
  readonly [XSymbol]: unknown;
}
export function consumeX(value: X) {
}

// package B
import { X } from "packageC";
export function createX(): X {
}

// package A
import { createX } from "packageB";
import { consumeX } from "packageC";
function main() {
  // this will give an error with the Symbol approach
  consumeX(createX());
}

In our case, we didn't want to go the lockstep and peer dependencies way, so we're not exporting symbols & classes to improve API compatibility between versions.

[GerardasB]

BTW, I think the problem of not being able to modify props-only or return-only interfaces in a non-breaking way is separate from the exported @internal APIs problem.

Regarding the latter, in our new packages we made sure we don't export any top-level @internal APIs through the barrel. However, we still have the problem with exported public interfaces exposing internal attributes.

I agree, AppUI issue to removing @internal APIs from the barrel file: https://github.com/iTwin/appui/issues/766 Strip-internals would be nice for cases where i.e. a public class has @internal properties.

Additionally, there are 2 React specific cases where not exporting an interface would help with API maintenance in minor versions (w/o breaking changes):

• Existing component props could be made optional & additional types can be added to unions https://github.com/iTwin/appui/issues/759
• A returned object from a hook should allow additional required properties (this issue is very similar https://github.com/iTwin/appui/issues/807)

As stated above, one of the main concerns with moving forward w/ these changes is the docs site.

[grigasp]

As stated above, one of the main concerns with moving forward w/ these changes is the docs site.

I would say that not having those props and return types both - in intellisense and the doc site - can be a good thing too. Here's an example:

export interface MyFunctionProps {
  p: number;
}
export interface MyFunctionReturnValue {
  r: number;
}
export function myFunction(props: MyFunctionProps): MyFunctionReturnValue {
  return { r: props.p };
}

With props & return value interfaces exported:

Not exported:

Not exporting those types makes the API surface less polluted - after all, consumers in this case care about the function and not those types. In the doc site I'd rather see them documented when I open the function docs page rather than as a completely separate API.

[GerardasB]

As stated above, one of the main concerns with moving forward w/ these changes is the docs site.

I would say that not having those props and return types both - in intellisense and the doc site - can be a good thing too. Here's an example:

Good point (MyFunctionProps would still have to be exported as that's used as an input w/o instantiator, while myFunction is an instantiator for ReturnValue w/o explicit type).

I like the symbol approach to ensure that the interface is instantiated w/ all the required properties, it makes most sense if the interface/type is used in multiple APIs OR is designed to be extended. Symbol Playground

[pmconne]

@aruniverse @calebmshafer @wgoehrig @ben-polinsky please review proposed strategy illustrated in #6783.

[pmconne]

@aruniverse @calebmshafer @wgoehrig @ben-polinsky please provide feedback on the proposed strategy illustrated in #6783.

[calebmshafer]

@pmconne I like the approach of using Symbols to help hide the the APIs from consumers that are members of an exported and public method.

Not exporting from barrel looks good for the internal folder but will need to be followed up with the strip-internals work to ensure these APIs still don't show up in intellisense.

The Symbol approach as @grigasp points out is problematic when you have a package that can't have multiple instances of it (such as core-backend and core-frontend). Although, we should try to enforce people do not have that anyone with a runtime check.

@grigasp I'm not sure I agree on the removal of the return interface structures that are output from the API. This would lead people to not know what it is and would potentially leading to them creating their own interface or set of props to define it. Therefore when we make an update they would have no idea to add it on their side.

Now I do agree in principle (and discussed with @wgoehrig) that most people should avoid strict typing and use type inference instead but I'm not sure we can rely on it.

Follow up action:

• Add an ESLint rule that will catch anyone trying to introduce a new API not following the rules.

[pmconne]

I'm not sure we can rely on it.

Given our goal is to never break compilation of apps that use public APIs when they upgrade to a minor or patch release, we can't. It lacks any enforcement mechanism.

[grigasp]

@grigasp I'm not sure I agree on the removal of the return interface structures that are output from the API. This would lead people to not know what it is and would potentially leading to them creating their own interface or set of props to define it. Therefore when we make an update they would have no idea to add it on their side.

• This would lead people to not know what it is

  How? The docs are there in intellisense and in case of doc site the return type can be documented next to the API returning it.

• and would potentially leading to them creating their own interface or set of props to define it. Therefore when we make an update they would have no idea to add it on their side.

  I don't see why consumers would need to add the new props to their types. If my function returns { x; y } and they have their own copy of this type, the assignment will just work. This still allows us to add additional required member z without breaking them and they can ignore the new member as long as they don't need it.

  Now if the type is not just return type, but also an input type to some of our other public function, then we're not allowed to add required members there, but we can still add optional ones. And that wouldn't break anything on consumers side.

[pmconne]

@aruniverse and @wgoehrig pointed out a potential pitfall with using unique symbols. If somebody's dependencies cause them to pull in multiple versions of, say, core-bentley, then core-bentley will contain two different sets of unique symbols. That would be a breaking change introduced in a minor release, even for well-behaved folks who are only using public APIs.

@wgoehrig proposed that for 4.x, we use const _close = Symbol.for("close") instead of const _close = Symbol() to put the symbols in the static registry and avoid that problem. In 5.0 we can enforce that no one depends on multiple versions of a single core package, and can switch to the unique symbol approach. 5.0, of course, would also be the window for more aggressively removing existing internal APIs.

In the interim, mischievous people would be able to sneakily call internal methods by looking up the corresponding symbol with Symbol.for. That's on them.

[pmconne]

A handful of internal methods in core-backend are invoked by native code, so cannot be changed to symbols. Many of these are private and I believe the rest can be changed to private, so we just need to be careful to identify them all.

[grigasp]

Not an issue to packages in itwinjs-core, but turns out symbols can't be used in React props interfaces. So for react packages this approach doesn't work.