[FIX] Extend views improve

4Source commented 2 months ago

As mentioned here https://github.com/Fevol/obsidian-typings/pull/80#issuecomment-2268491508 There are some issues with the latest update.

Changes

I updated the return types of the Views to not only return the expected fix string like "markdown" to also return string to fix the overriding issue
I updated the interface to class for the internal views so it is possible to use the extends keyword on them

4Source commented 2 months ago

It seems like not that easy as just change the views to class this makes them the implementation and you get an error when trying to extends

TypeError: Class extends value undefined is not a constructor or null

4Source commented 2 months ago

While searching for a solution I came across this https://www.typescriptlang.org/docs/handbook/declaration-files/by-example.html This is a guide to writing declaration files and the use of the declaration statement (declare) is missing here

mnaoumov commented 2 months ago

@4Source you cannot have classes in obsidian-typings, only interfaces. If you need to define class, you have to put them in src/obsidian/implementations and use via obsidian-typings/implementations

mnaoumov commented 2 months ago

It seems like not that easy as just change the views to class this makes them the implementation and you get an error when trying to extends
TypeError: Class extends value undefined is not a constructor or null

You get this issue because you don't have an instance of base class to extend. That could be done only via implementations (see my comment above)

mnaoumov commented 2 months ago

While searching for a solution I came across this https://www.typescriptlang.org/docs/handbook/declaration-files/by-example.html This is a guide to writing declaration files and the use of the declaration statement (declare) is missing here

This will not help because besides declaration we need some implementation and as we agreed with @Fevol , we will add implementations only to obsidian-typings/implementations

4Source commented 2 months ago

@4Source you cannot have classes in obsidian-typings, only interfaces. If you need to define class, you have to put them in src/obsidian/implementations and use via obsidian-typings/implementations

The thing is I don't want to add "real" classes but the views are implemented as classes and if you annotate them as interfaces they behave differently e.g. implements class MyView implements PdfView {} requires to implement alle functions of PdfView by my self and those are a lot

It seems like not that easy as just change the views to class this makes them the implementation and you get an error when trying to extends
TypeError: Class extends value undefined is not a constructor or null
You get this issue because you don't have an instance of base class to extend. That could be done only via implementations (see my comment above)

What I have seen typescript adds the declarations based on the compatible structure there are existing and those classes exist in obsidian

mnaoumov commented 2 months ago

@4Source obsidian doesn't have PdfView class exported

require("obsidian").PdfView returns undefined.

Meaning that if you want to use it as a class to extend it, you need to have an access to its implementation, either define it on your own or find a way how to reach it from obsidian such as require("obsidian").someProperty.someMethod().PdfView

4Source commented 2 months ago

@4Source obsidian doesn't have PdfView class exported

require("obsidian").PdfView returns undefined.

Meaning that if you want to use it as a class to extend it, you need to have an access to its implementation, either define it on your own or find a way how to reach it from obsidian such as require("obsidian").someProperty.someMethod().PdfView

Isn't this the whole reason for this project to allow access to not exported implementations?

mnaoumov commented 2 months ago

@4Source not really

The aim of the project is to provide typings to obsidian internals, but not the implementations. Even more, you in your comment https://github.com/Fevol/obsidian-typings/issues/63#issuecomment-2143822341 even voted against adding implementations to this project. However, we decided to still add some basic implementations and I have a separate project for more advanced implementations (WIP).

4Source commented 2 months ago

The aim of the project is to provide typings to obsidian internals, but not the implementations.

I am not sure I think we talk about different things PdfView is an view implemented by OBSIDIAN like FileView, View or EditableFileView we don't implemented them but we added typing for them. So why should this PdfView be a implementation.

I can't get it may I miss something...

Even more, you in your comment #63 (comment) even voted against adding implementations to this project. However, we decided to still add some basic implementations and I have a separate project for more advanced implementations (WIP).

I know and I still think this way

mnaoumov commented 2 months ago

FileView, View or EditableFileView we don't implemented them but we added typing for them. So why should this PdfView be a implementation.

FileView is exposed by obsidian
View is exposed by obsidian
EditableFileView is exposed by obsidian
PdfView is NOT exposed by obsidian. That's why we need to treat it separately. See https://github.com/Fevol/obsidian-typings/pull/83#issuecomment-2269175711

Fevol commented 2 months ago

Here is my view on things: obsidian-typings core goal should state the types of interfaces, methods etc, regardless of whether they are exported or available only through implementations.

The problem with views like PdfView is indeed that there is no straight-forward way to construct your own view to use the typings library for. The easiest way, to my knowledge, is to create a mock view and grab its prototype (example: my code).

So how should obsidian-typings help developers actually use these kinds of views in their projects?

Don't add typings to unexported interfaces and classes → This wouldn't be very useful to developers, as they'd have to type them themselves.
Actually re-implement the view class and expose as the internals package → The implementation of the view could change after any update, it would be quite a bit of work to maintain, and it'd be better not to have deobfuscated Obsidian source code public.
Provide a method and guide to get these kinds of views → In my eyes, this is the best option. This would be the least effort for both us and the users of the package. The (WIP) website would be great to give an overview of how to actually get the implementations.

As for whether /implementations should be included with this package or not, my opinion on that is simple: keep the implementations bundled with this package as long as it is convenient for the users of this package. (I.e. no separate package installs, everything exists under one umbrella)

If there are benefits to offshooting the implementations into their own separate package, then I'd absolutely be open to discussing moving them. As far as I understand, the only real problem with the current approach, is that the current method for non-nodenext users to access these implementations, is a slight hack.

Also replying to the discussion in #80. obsidian-typings (the typings) should export interfaces, not classes. Making them classes might give users the idea that they are actual implementations that can be used, rather than just types.

@4Source, @mnaoumov If there is anything I missed in the discussions, please let me know. I want to make sure that we're all on the same page. (And tell me if I am mistaken or misunderstanding something)

4Source commented 2 months ago

I think I get it now the problem is in the source code for all those views I added there don't have this in the obsidian source code

I thought it is somehow possible to to match the typing to the classes in obsidian

mnaoumov commented 2 months ago

@4Source let me clarify something

interface and type are things that exist only in TypeScript and are excluded from the compiled JavaScript. So in runtime they don't exist.

From the other hand class exists in JavaScript and runtime, they are actually functions. If you run typeof MyClass you will get "function".

When you do class MyChildClass extends MyClass {}, you have to have MyClass available in runtime.

Everything we do in obsidian-typings exists only in compile-time.

As obsidian doesn't expose PdfView easily, we need to find a way how to get access to its implementation. It might be tricky like in the link above

If you can find the way to reach them, you can implement it as a custom implementation. See, for example, createTFileInstance. It's an interesting case, because even though TFile is exposed, but its constructor is not, and I had to come up with the custom implementation.

I guess in your case you will end up with

export function getPdfViewConstructor(app: App): Constructor<PdfView> {
  // your very sophisticated implementation
}

then in your code

export default class DrawablePDFView extends createPdfViewConstructor(app) {
}

mnaoumov commented 2 months ago

@4Source I am doubtful about this PR. I don't think it makes much sense to have "value" | string. Let's wait until you get a real fully working example of the newly added views

4Source commented 2 months ago

I am trying to get this work for this I created a function but I can get the build implementation get to run the types.d.ts get generated properly but it seems to stop by build implementations

PS D:\Documents\Projekte\Aktuell\obsidian-typings> npm run build

> obsidian-typings@2.2.1-beta.8 build
> npm-run-all build:clean build:validate build:generate-index build:bundle-types build:implementations build:implementations:generate-types build:implementations:bundle-types build:style-mod build:extract-api

> obsidian-typings@2.2.1-beta.8 build:clean
> bun run scripts/build-clean.ts

> obsidian-typings@2.2.1-beta.8 build:validate
> tsc --build --force

> obsidian-typings@2.2.1-beta.8 build:generate-index
> bun run scripts/build-generate-index.ts

> obsidian-typings@2.2.1-beta.8 build:bundle-types
> dts-bundle-generator -o ./dist/types.d.ts ./src/index.d.ts --inline-declare-global --inline-declare-externals

Compiling input files...
Processing ./src/index.d.ts
Writing ./src/index.d.ts -> ./dist/types.d.ts
Checking generated files...
Done in 13.55s

> obsidian-typings@2.2.1-beta.8 build:implementations
> bun run scripts/build-implementations.ts

EDIT: Is a specific version of bun requiered?

PS D:\Documents\Projekte\Aktuell\obsidian-typings> bun -v
1.0.30

4Source commented 2 months ago

The problem with views like PdfView is indeed that there is no straight-forward way to construct your own view to use the typings library for. The easiest way, to my knowledge, is to create a mock view and grab its prototype (example: my code).

@Fevol This seems not to work anymore with latest version of obsidian there removed the deprecated app now completely. Latest version: Older version:

any ideas for a Workaround? Could be pretty hard without the global app instance

Fevol commented 2 months ago

I am trying to get this work for this I created a function but I can get the build implementation get to run the types.d.ts get generated properly but it seems to stop by build implementations

Can't reproduce this issue on the latest version (beta-8). I have no idea why it would even get stuck on that part (have you tried running it on node, reinstalling the packages?)

This seems not to work anymore with latest version of obsidian there removed the deprecated app now completely. (...) any ideas for a Workaround? Could be pretty hard without the global app instance

The best solution would probably be to return the class as a function, and then determine it on plugin load. Alternatively, for this use-case, it might be fine to just keep using app.

4Source commented 2 months ago

Can't reproduce this issue on the latest version (beta-8).

main branch also not working for me

(have you tried running it on node, reinstalling the packages?)

how could I run .ts with node?

I have no idea why it would even get stuck on that part

const context = await esbuild.context({

this statement seems not to get finished for me

The best solution would probably be to return the class as a function, and then determine it on plugin load.

not sure what you mean

Alternatively, for this use-case, it might be fine to just keep using app.

how it is no longer available

mnaoumov commented 2 months ago

@4Source

I can't reproduce your build issue

Tried on bun 1.1.10, ts-node 10.9.2, tsx 4.16.3:

bun ./scripts/build-implementations.ts

npm install ts-node
node --loader ts-node/esm ./scripts/build-implementations.ts

tsx ./scripts/build-implementations.ts

I added global variable app back in v2.2.1-beta.9
We can avoid using global app via using extra level of nesting

function getEmbeddableMarkdownEditorClass(app: App): Constructor<MarkdownScrollableEditView> {
  return class EmbeddableMarkdownEditor extends resolveEditorPrototype(app) implements MarkdownScrollableEditView {
    //
  };
}

4Source commented 2 months ago

I updated bun to 1.1.x now it is working

Fevol / obsidian-typings

[FIX] Extend views improve #83

Changes