[BUG] typing files and real code are different

[dilame]

Context:

• Playwright Version: 1.4.2

It is not really a bug, but it is a problem. You are using TypeScript. Me too. But it looks like you are writing the types.d.ts manually, and it is different from the actual code. You are using classes, for example - browser. But in your types.d.ts it is declared as interface Browser.
The main issue - as i mentioned, i use TypeScript, and i use the Dependency Injection technique. It is very convenient when you declare it like this

import {Browser} from 'playwright';

@Service()
export class SomeService {
  constructor(private browser: Browser) {}
} 

i hope you know how TypeScript reflection works, and understand that if Browser is declared as interface - the reflection doesn't work, so i am forced to use workarounds.

Your sources are in TypeScript, so why not just let tsc generate proper typings automatically?

[JoelEinbinder]

The short answer is that we don't export classes as part of our public api, so this technique will not work with playwright. This is unrelated to our manual typings.

We've had good reasons for not exporting classes. All of them require a private async initialization step, so the constructors are meaningless. On multiple times we've switched things from being a class to an interface with multiple implementations. If we had exported classes, these would have been breaking changes.

TypeScript metadata is an interesting new argument in favor of exporting classes, but by itself I don't think its worth it. The browser class constructor doesn't contain enough information to properly construct the browser object. You are going to have to manually write the construction code, and then you might as well just wrap it in a function that asynchronously returns a singleton.

If you have some context as to the root problems you are trying to solve, I think the https://github.com/microsoft/playwright-test project would be interested in solving them with fixtures.

[JoelEinbinder]

Closing as wontfix, but feel free to continue the discussion here or in playwright-test.

[dilame]

You are going to have to manually write the construction code, and then you might as well just wrap it in a function that asynchronously returns a singleton.

Yes, and it is ok. The main idea of the Dependency injection technique is not a magical constructor calling, but resolving the declared dependency from the IoC container. It doesn't matter how objects appear in this container, most of the time it is complex async object building, just like yours. Nowadays most of the required services are constructed asynchronously (database connection, external HTTP API with authorization, MQ's, etc), and it can not be the reason to avoid the DI technique, because the consumer class just needs the initialized object, it doesn't care how and when it was initialized. Most dependencies are initialized before the application starts.

If you don't want users to call the constructor - you can declare it as private.

Manual typings are the distortion of reality. There are at least two signs that this is the wrong approach:

1. You are writing it by your hands, distorting the reality, while tsc can perfectly do it automatically, reflecting the reality.
2. I can't use the classes as DI tokens. Kinda Schrodinger classes. They exist and not exist at the same time.

This is uncertainty, which is inherently bad.

I can understand that you are trying to protect users from the mistakes, but, for real, we are not kids, we are developers (sometimes it's the same person, but... 😄 ). If i use the class (even if this class was returned from await firefox.launch), then this class is already the part of public API, you can not change this fact by not exporting it from the root. Moreover - not exporting it from the root becomes a confusion - why am i using some class but can not import it?

[dilame]

i really hope we could re-open this issue

[dilame]

It will make life easier not only for me as a user but for you as maintainers. Because having manually written typing files while you have original TypeScript code is a violation of the universal Single source of truth practice. Every time you make 1 change you should make it in 2 places.

If we had exported classes, these would have been breaking changes.

Why? Nothing will break if you will export something, that you have not exported before.

[dilame]

You are telling

we don't export classes as part of our public api

But your documentation declares class: Browser (Just like all the other entities in your API reference)

https://playwright.dev/#version=v1.4.2&path=docs%2Fapi.md&q=class-browser

If it is not a public API, then what is it?

[dilame]

Another shortcoming is the impossibility to check the concrete browser instance.

For example, i accept the abstract Browser created outside my module. But if user passed ChromiumBrowser, i want to check it and handle a little differently.

export class MyModule {
  constructor(browser: Browser) {
    // Impossible to check
    if(brower instanceof ChromiumBrowser) {
      doSomethingSpecial()
    }
  }
}

@pavelfeldman @dgozman please, pay attention to this issue.

[dgozman]

For example, i accept the abstract Browser created outside my module. But if user passed ChromiumBrowser, i want to check it and handle a little differently.

I know this is not what you are asking for, but we can introduce Browser.name() similar to BrowserType.name(), so that you can compare it to "chromium".

On the topic of exporting the classes: are there any other usecases except for using them as DI tokens? I'd like to understand pros vs cons of exposing the classes. Perhaps, there are more folks interested in using our classes for DI?

[dilame]

we can introduce Browser.name() similar to BrowserType.name(), so that you can compare it to "chromium".

Yes, but... why to do this workaround while JavaScript offers a standard way to check if an object is an instance of some class - there is a special instanceof operator.
Just compare

const browser = await firefox.launch();

// Looks strange, like tradeoff
if(browser.name === 'firefox') {}

// Reads like speech - if browser instance of firefox
if(browser instanceof FirefoxBrowser) {}

I think it is obvious that we should utilize the native language features instead of intent our own. Moreover, i would understand if you have some reasons to search for a compromise workaround, but you already using real human classes which we can use like we are bosses. So i don't understand why reinvent the wheel.

So, for now, i have 3 main arguments in favor of exporting classes:

1. DI tokens.
2. instanceof checking
3. Reflection of reality. Sounds easy, but most important - you are using classes in your source code. Why hide it behind manual interfaces?

I already described these arguments in detail. I really don't understand your reasons to hide the classes.

[dilame]

I'd like to understand pros vs cons of exposing the classes

i mention 5 cons

1. DI tokens.
2. instanceof checking
3. Reflection of reality
4. Your official documentation makes me believe that there are classes.
5. Automatic types generation, in accordance with modern TypeScript standards. Nobody writes manual typings when the sources code written in TypeScript.

Can someone tell any cons?

[dilame]

Guys 😢 ?

[dilame]

Is there any updates?

[dilame]

@pavelfeldman @aslushnikov @yury-s @arjunattam @mxschmitt @kblok @rwoll @domderen