Extract into individual files

mnaoumov commented 4 months ago

Fixes #63

Fevol commented 4 months ago

Thanks for the great work, this looks absolutely fantastic!

I am however encountering some issues:

I'm getting a type issue with importing (d).ts files (e.g. for obsidian/Vault.d.ts. I've tried changing the tsconfig.json, but haven't found the correct set of configurations that fixes the issue.)
TS language server seems to not be able to pick up the typings anymore. (Which is probably related to the previous problem)

I don't know if you are also facing these issues, it might be a particular configuration problem with the repository I use for testing the typings.

mnaoumov commented 4 months ago

@Fevol I made it fully working on my test repository.

Can you please share the test repository where you encounter misbehavior?

mnaoumov commented 4 months ago

@Fevol let me clarify

Are you trying to

import "obsidian/Vault.d.ts";

?

If so, I didn't consider this scenario. You want to be able to augment only individual interfaces without modifying others?

Fevol commented 4 months ago

Can you please share the test repository where you encounter misbehavior?

Sadly not easily, I did try to replace the obsidian-ex.d.ts with the lib directory for another repository (e.g. used as a NPM package in node_modules), and that worked without issue. So it definitely has something to do with how I configured my test repository.

Are you trying to (...)

No, in this case, the problem is just having the typings be registered to the language server (i.e. App not being extended). I am using a slightly unorthodox set-up with symlinks, so I am wondering whether that has something to do with it:

├─ \source_code\ (Source code for reverse-engineering)
├─ \obsidian-typings\ (Git repo for developing the typings, SYMSOURCE)
└─ \plugin_test\ (Sample plugin)
      └─ \node_modules\
             └─ \@types\
                    └─ \obsidian-typings\ (SYMLINKED)

I'll try to see if I can find the issue ASAP.

mnaoumov commented 4 months ago

@Fevol your test environment looks similar to mine

Try the following

cd /path/to/obsidian-typings
npm link

cd /path/to/plugin_test
npm link obsidian-typings

And try again

Fevol commented 4 months ago

@mnaoumov Hmm, that doesn't seem to have made a difference. One other thing I noticed is: TS2305: Module "obsidian-typings" has no exported member InternalPluginName when I try to import { InternalPluginName } from "obsidian-typings";

mnaoumov commented 4 months ago

@Fevol I am currently doing some final changes, and updating the docs to provide examples

Fevol commented 4 months ago

Alright, I finally managed to resolve the issue. The LSP was using the typescript package from v4.7.5, when I installed typescript for the obsidian-typings repository and changed the LSP to that typescript install, it worked again.

So after updating typescript for plugin_test from v4.7.5 to v5.4.5 and setting the LSP to that instance, the issue went away and the typings got picked up again. I'm guessing some particular config was changed between those two versions that caused the issue (or perhaps something with ESNext? Haven't investigated it yet.

mnaoumov commented 4 months ago

@Fevol hard to say, as I am not very familiar with older versions of TypeScript, however I did change the settings to the most modern ESNext and NodeNext. It is more future-proof

mnaoumov commented 4 months ago

@Fevol ready to test

Fevol commented 4 months ago

Apologies for the wait, I spent a while reworking the custom tsmorph formatter to the new directory structure and the formatter you are using (i.e. neatly ordered imports/exports) -- I hope it is alright that I've added those to this PR too.

Further, I'm sorry that I won't be able to merge the PR immediately. I spent the past two hours trying to figure out why the typings aren't getting picked up again for both my sample and 'real' plugins. Eventually though, I did manage to get things working, some takeaways/observations from my experience:

The next version should definitely be a 2.0.0 version, as there are quite a lot of breaking changes:

From 5a835238 onwards, there is also an issue where the definition file for obsidian-typings cannot be found (TS2688), I haven't investigated this yet, but I did notice that some imports were erroneously renamed to XXXX.d.ts, so it might have something to do with that.
Some interfaces/classes obviously need to be imported from obsidian-typings instead of obsidian itself (I realized this far too late, because dozens of different errors got spat out 😅)
Plugin's tsconfig.json now requires you to add allowImportingTsExtensions: true (Would it be possible to avoid having this requirement? The errors can be very vague when users update without knowing what changed.)
Importing from obsidian-typings doesn't give autocompletions for the classes, only for the plugin values (InternalPluginName).
Extending types further (e.g. in a extensions.d.ts file within the plugin repo) does not seem to work anymore; the typings there take precendence over the types defined in obsidian-typings. Specific example:
```
// extensions.d.ts
declare module "obsidian" {
interface Plugins {
    plugins: {
        "commentator": CommentatorPlugin;
    };
}
}
```

// main.ts this.remove_monkeys.push(around(this.app.plugins, { // ERROR: Plugins does not have a method uninstallPlugin uninstallPlugin: (oldMethod) => { return async (id: string) => { oldMethod && await oldMethod.apply(this.app.plugins, [id]); if (id === "commentator") await this.database.dropDatabase(); }; }, }));


As it is already quite late now, I will try look at the issues in more detail tomorrow. It is very likely that I made some stupid mistake somewhere along the line; so I'll need to look at it with a fresh mind.

If at all possible, I would like to make the migration for dependants of this package smooth enough that it doesn't require more than five minutes of work. The main problem will be communication of the changes, I'll probably make a Discord post and a pinned issue on this repo so as many people will see it as possible.

I want to note though: I am really happy with these changes! I'll admit - at first I was doubtful, but having each interface separated really does make navigation and browsing much easier. Thank you very much for all your fantastic work, and sorry for all the hold-ups!

mnaoumov commented 4 months ago

@Fevol thanks for the feedback

I hope it is alright that I've added those to this PR too.

That's great, so you can force the code style before accepting PRs.

I spent the past two hours trying to figure out why the typings aren't getting picked up again for both my sample and 'real' plugins

It also took me way more hours to understand how to get it fully working. Now I know about typescript compilation, type resolutions etc, way more than I ever wanted to.

The next version should definitely be a 2.0.0 version

Agree

as there are quite a lot of breaking changes:

that's hopefully a misunderstanding

From https://github.com/Fevol/obsidian-typings/commit/5a8352385712f32ee232eb129682c749829e8fad onwards, there is also an issue where the definition file for obsidian-typings cannot be found (TS2688)

I will try to replicate this issue from the new project, as my current ones works perfectly

some imports were erroneously renamed to XXXX.d.ts

Why do you think it is an error? It is actually a very deliberate choice

Some interfaces/classes obviously need to be imported from obsidian-typings instead of obsidian itself

Yes, this is indeed a breaking change, but I think it totally worth it. In my opinion previous model is extremely confusing and misleading.

Plugin's tsconfig.json now requires you to add allowImportingTsExtensions: true (Would it be possible to avoid having this requirement? The errors can be very vague when users update without knowing what changed.)

this setting is only required for the project itself, and should not be forced to the client projects

I honestly think that without this setting the code looks horrible.

It forces to write

import { InternalPlugins} from "../types.js";

while there is no types.js file. I think it is extremely misleading and unfriendly.

There is one place in lib/types.d.ts where I couldn't avoid making reference to non-existing files. I made me upset, but it is what it is. I put a comment there.

Importing from obsidian-typings doesn't give autocompletions for the classes, only for the plugin values (InternalPluginName).

That's very strange, because in my latest fix, I clearly separated types vs implementations, see README

Extending types further (e.g. in a extensions.d.ts file within the plugin repo) does not seem to work anymore; the typings there take precendence over the types defined in obsidian-typings. Specific example:

Thanks, I will test your example shortly

As it is already quite late now, I will try look at the issues in more detail tomorrow. It is very likely that I made some stupid mistake somewhere along the line; so I'll need to look at it with a fresh mind.

I will try to help you to get this tested ASAP. Don't hesitate to ask. I am in the UTC-6 timezone

If at all possible, I would like to make the migration for dependants of this package smooth enough that it doesn't require more than five minutes of work. The main problem will be communication of the changes, I'll probably make a Discord post and a pinned issue on this repo so as many people will see it as possible.

I would create a MIGRATION.md document or similar describing how to do the migration, step-by-step, but, honestly, I don't think there should be many.

I want to note though: I am really happy with these changes! I'll admit - at first I was doubtful, but having each interface separated really does make navigation and browsing much easier. Thank you very much for all your fantastic work, and sorry for all the hold-ups!

I am a big fan of your project and would be very proud if you can take advantage of my efforts!

mnaoumov commented 4 months ago

@Fevol I can confirm I am having similar issues to the ones you described, when I tried to use the new version in my other project. I will troubleshoot and find the solution

mnaoumov commented 4 months ago

@Fevol it took me awhile but I figured it out

Troubleshooting

The most difficult part was to understand why the typings are not picked up while compilation works. So the trick is to set "skipLibCheck": false in the test project, and run tsc --build --force and it will help to catch the issue instantly.

Incompatible `obsidian` package

My test project had old version of obsidian installed which is not compatible with the latest one used by obsidian-typings. We set it a peerDependency but it seems to be not good enough

So please ensure your obsidian package is updated.

`"moduleResolution": "NodeNext"`

Please ensure your tsconfig.json has the following setting. As of now, obsidian-typings is built for this mode, and in case we want to support other older systems, we can build sources into the format expected by older systems.

I am more than welcome to help you with this, but I think first we need to ensure it works at least for you, before we move any on :)

Fevol commented 4 months ago

Why do you think it is an error? It is actually a very deliberate choice

I was thinking that because I saw a package.json being renamed to package.d.tson in one of the scripts (https://github.com/Fevol/obsidian-typings/commit/5a8352385712f32ee232eb129682c749829e8fad#diff-d7c10983349f520dd101e4bcb29702f993cf43b625cc0128be48eb8f80282957L31-R32), but I am guessing it's just an isolated problem

Yes, this is indeed a breaking change, but I think it totally worth it. In my opinion previous model is extremely confusing and misleading.

Absolutely, this is definitely necessary, I'll just need to communicate it well so people won't get confused!

I applied all of the fixes but I am still hitting some roadblocks:

Extending from tsconfig.jsons provided by packages is an issue (example: Svelte requires you to extend its tsconfig if you want to have TS typings for svelte files)
Even after removing the extend, the tsc --build --force still returns Cannot find type definition file for 'obsidian-typings'. without any other indications of what the issue is.

mnaoumov commented 4 months ago

@Fevol I created a new repo where we can test the build

https://github.com/mnaoumov/obsidian-typings-build-test

I gave you the write permissions, so we can work on this problem more efficiently.

Added both examples that you mentioned.

The biggest gotcha, in my opinion, is https://github.com/mnaoumov/obsidian-typings-build-test/blob/master/project1/extensions.d.ts#L4

4Source commented 3 months ago

Any progress on this?

Fevol commented 3 months ago

@mnaoumov, @4Source Apologies for the delay. I have some final notes on this PR, then it is good to merge for me:

Questions

While everything now functions properly on my set-up with moduleResolution: bundler, I have not been able to get moduleResolution: NodeNext to work at all. Does this work on your end, or is this option not necessary?
Similarly, does the configuration with "moduleResolution": "bundler" or types: ... work in your IDE's/set-ups?

Potential Changes

Allowing legacy types importing

Instead of exclusively using exports in package.json to expose the types in the library:

{
    "name": "obsidian-typings",
    "exports": {
        ".": "./lib/types.d.ts",
        "./implementations.js": "./lib/implementations.ts"
    },
}

It might be interesting to also include a types field to allow for people with a TypeScript version lower than 5 to still use the library, as this is still the default in the sample plugin:

{
    "...": "...",
    "types": "./lib/types.d.ts",
}

This also makes the migration completely painless (save for having to change a few imports).

However, while it does seem to compile without issue, my IDE's language server seems to only sporadically be able to provide type hints for the added typings.

Removing `.js` extension from imports

In my set-up, at least, having the .js extension included for the imports within the library itself, does not seem to make any difference. I did read somewhere that they are necessary when using NodeNext resolution, so if it is not possible to remove then, that's completely fine.

Migration Guide

For me, it is important that the upgrade to the new version is as smooth as possible. I tried noting down the most important pitfalls below, is there anything I'm missing or anything that should be changed?

Minimal changes

Migrating to the new version (v2.0.0) will require at minimum the following changes (if types: ... is not specified):

typescript needs to be updated to 5.0.0 or higher, the sample plugin currently only specifies 4.7.4, so it is highely likely that you need to upgrade (this can be easily done by running npm install typescript@latest)
tsconfig.json needs to be updated to have moduleResolution: bundler instead of moduleResolution: node, this is now required in order to properly resolve the library.
Importing undocumented methods now requires you to import from obsidian-typings, and not just obsidian. This is done in order to make it more apparent that you are making use of undocumented and possibly volatile API. See below for the full list of interfaces that should be imported as import { X } from "obsidian-typings instead of import { X } from "obsidian".

Full list of classes that need to be imported from obsidian-typings

``` AbstractSearchComponent Account AppMenuBarManager AppVaultConfig BaseEditor CanvasConnection CanvasLeaf CanvasNode CanvasView ClipBoardManager Commands ConfigItem CustomArrayDict CustomCSS DragManager EditorSearchComponent EditorSuggests EmbedContext EmbeddableConstructor EmbeddedEditorView EmbedRegistry FileCacheEntry FileEntry FileExplorerLeaf FileExplorerView FileSuggest FileSuggestManager FileTreeItem FoldInfo FoldManager FootnoteCache GlobalSearchLeaf HotkeyManager HotkeysSettingTab HoverLinkEvent IFramedMarkdownEditor ImportedAttachments InfinityScroll InternalPlugin KeyScope LeafEntry LinkUpdate LoadProgress MarkdownBaseView MarkdownScrollableEditView MetadataEditor MetadataEditorProperty MetadataTypeManager MetadataWidget MobileNavbar MobileToolbar ObsidianDOM ObsidianTouchEvent Plugins PluginUpdateManifest PositionedReference PropertyEntryData PropertyInfo PropertyRenderContext PropertyWidget PropertyWidgetType ReadViewRenderer RecentFileTracker RendererSection Runnable SearchCursor SerializedWorkspace StateHistory SuggestionContainer TableCell TableCellEditor TableEditor ThemeManifest Token Tree TreeItem TreeNode ViewRegistry WeakMapWrapper WidgetEditorView WindowSelection ```

Extending Plugins interface with your own plugin

If you used to have the code below for registering your plugin to the app.plugins object:

import { Plugin } from "obsidian";
declare module "obsidian" {
    interface Plugins {
        plugins: Record<string, Plugin> & {
            "your-plugin": YourPlugin;
        };
    }
}

You should instead use:

import { Plugins as PluginsInterface } from "obsidian-typings";
declare module "obsidian" {
    export interface Plugins extends PluginsInterface {
        plugins: PluginsInterface["plugins"] & {
            "your-plugin": YourPlugin;
        };
    }
}

Importing from `/implementations`

If you want to import from implementations (which contain methods such as createTFile, createTFolder, CustomArrayDict and InternalPluginName):

Method 1. If obsidian-typings was installed using an alias (i.e. package is installed in @types/obsidian-typings), you must alias the implementations separately within your tsconfig.json (baseUrl should also be set):

{
  "compilerOptions": {
    "baseUrl": ".",
    "paths": {
      "obsidian-typings/implementations": [
        "../node_modules/@types/obsidian-typings/lib/implementations"
      ]
   }
}

Then the methods can be imported as:

import { InternalPluginName } from "obsidian-typings/implementations";

console.log(InternalPluginName.FileExplorer);

Method 2. (NOTE: I have not been able to get this to work myself) You can also import directly from the module using:

import { InternalPluginName } from "./node_modules/@types/obsidian-typings/lib/implementations.js";

console.log(InternalPluginName.FileExplorer);

mnaoumov commented 3 months ago

@Fevol Please see my replies inline

@mnaoumov, @4Source Apologies for the delay. I have some final notes on this PR, then it is good to merge for me:

Questions

While everything now functions properly on my set-up with moduleResolution: bundler, I have not been able to get moduleResolution: NodeNext to work at all. Does this work on your end, or is this option not necessary?

Please loot at my comment https://github.com/Fevol/obsidian-typings/pull/65#issuecomment-2147877581

I created a repo for you and me to identify all the migration pains.

Please create there an example where "moduleResolution": "bundler" is working and "moduleResolution": "NodeNext" doesn't. I will be more than happy to get it fixed.

Similarly, does the configuration with "moduleResolution": "bundler" or types: ... work in your IDE's/set-ups?

I never tested "moduleResolution": "bundler" setting and I think that supporting "moduleResolution": "NodeNext" is better because it is stricter.

Potential Changes

Allowing legacy types importing

Instead of exclusively using exports in package.json to expose the types in the library:
{
    "name": "obsidian-typings",
    "exports": {
        ".": "./lib/types.d.ts",
        "./implementations.js": "./lib/implementations.ts"
    },
}
It might be interesting to also include a types field to allow for people with a TypeScript version lower than 5 to still use the library, as this is still the default in the sample plugin:
{
    "...": "...",
    "types": "./lib/types.d.ts",
}
This also makes the migration completely painless (save for having to change a few imports).

Sure, let's add types as well.

However, while it does seem to compile without issue, my IDE's language server seems to only sporadically be able to provide type hints for the added typings.

This could require you to restart TypeScript server, but this usually happens only one time.

Removing .js extension from imports

In my set-up, at least, having the .js extension included for the imports within the library itself, does not seem to make any difference. I did read somewhere that they are necessary when using NodeNext resolution, so if it is not possible to remove then, that's completely fine.

Having extensions is mandatory for NodeNext module resolution (to be precise, starting from Node16), so I strongly recommend we keep it that way for future compatibility. I already gave on the idea to keep .ts extensions and have to switch to the non-existing .js ones, but unfortunately we have to do it to support more client configurations.

Migration Guide

For me, it is important that the upgrade to the new version is as smooth as possible. I tried noting down the most important pitfalls below, is there anything I'm missing or anything that should be changed?

Minimal changes

Migrating to the new version (v2.0.0) will require at minimum the following changes (if types: ... is not specified):

We don't have reasons not to specify types.

typescript needs to be updated to 5.0.0 or higher, the sample plugin currently only specifies 4.7.4, so it is highely likely that you need to upgrade (this can be easily done by running npm install typescript@latest)

I will investigate if this is indeed required.

tsconfig.json needs to be updated to have moduleResolution: bundler instead of moduleResolution: node, this is now required in order to properly resolve the library.

I will investigate if this is indeed required.

Importing undocumented methods now requires you to import from obsidian-typings, and not just obsidian. This is done in order to make it more apparent that you are making use of undocumented and possibly volatile API. See below for the full list of interfaces that should be imported as import { X } from "obsidian-typings instead of import { X } from "obsidian".

Full list of classes that need to be imported from obsidian-typings

AbstractSearchComponent
Account
AppMenuBarManager
AppVaultConfig
BaseEditor
CanvasConnection
CanvasLeaf
CanvasNode
CanvasView
ClipBoardManager
Commands
ConfigItem
CustomArrayDict
CustomCSS
DragManager
EditorSearchComponent
EditorSuggests
EmbedContext
EmbeddableConstructor
EmbeddedEditorView
EmbedRegistry
FileCacheEntry
FileEntry
FileExplorerLeaf
FileExplorerView
FileSuggest
FileSuggestManager
FileTreeItem
FoldInfo
FoldManager
FootnoteCache
GlobalSearchLeaf
HotkeyManager
HotkeysSettingTab
HoverLinkEvent
IFramedMarkdownEditor
ImportedAttachments
InfinityScroll
InternalPlugin
KeyScope
LeafEntry
LinkUpdate
LoadProgress
MarkdownBaseView
MarkdownScrollableEditView
MetadataEditor
MetadataEditorProperty
MetadataTypeManager
MetadataWidget
MobileNavbar
MobileToolbar
ObsidianDOM
ObsidianTouchEvent
Plugins
PluginUpdateManifest
PositionedReference
PropertyEntryData
PropertyInfo
PropertyRenderContext
PropertyWidget
PropertyWidgetType
ReadViewRenderer
RecentFileTracker
RendererSection
Runnable
SearchCursor
SerializedWorkspace
StateHistory
SuggestionContainer
TableCell
TableCellEditor
TableEditor
ThemeManifest
Token
Tree
TreeItem
TreeNode
ViewRegistry
WeakMapWrapper
WidgetEditorView
WindowSelection

Extending Plugins interface with your own plugin

If you used to have the code below for registering your plugin to the app.plugins object:

import { Plugin } from "obsidian";
declare module "obsidian" {
  interface Plugins {
      plugins: Record<string, Plugin> & {
          "your-plugin": YourPlugin;
      };
  }
}

You should instead use:

import { Plugins as PluginsInterface } from "obsidian-typings";
declare module "obsidian" {
  export interface Plugins extends PluginsInterface {
      plugins: PluginsInterface["plugins"] & {
          "your-plugin": YourPlugin;
      };
  }
}

I disagree with your suggestion either. I think this is the way to go: https://github.com/mnaoumov/obsidian-typings-build-test/blob/master/project1/extensions.d.ts

Importing from /implementations

If you want to import from implementations (which contain methods such as createTFile, createTFolder, CustomArrayDict and InternalPluginName):

Method 1. If obsidian-typings was installed using an alias (i.e. package is installed in @types/obsidian-typings), you must alias the implementations separately within your tsconfig.json (baseUrl should also be set):

Actually baseUrl SHOULD NOT be set, it is a bad confusing thing, because it breaks the relative paths and auto-complete. I would strongly advise to remove it instead.

{
  "compilerOptions": {
    "baseUrl": ".",
    "paths": {
      "obsidian-typings/implementations": [
        "../node_modules/@types/obsidian-typings/lib/implementations"
      ]
   }
}

"baseUrl": "." -> remove
"../node_modules/@types/obsidian-typings/lib/implementations" -> "node_modules/@types/obsidian-typings/lib/implementations"

Then the methods can be imported as:
import { InternalPluginName } from "obsidian-typings/implementations";

console.log(InternalPluginName.FileExplorer);
Method 2. (NOTE: I have not been able to get this to work myself) You can also import directly from the module using:
import { InternalPluginName } from "./node_modules/@types/obsidian-typings/lib/implementations.js";
console.log(InternalPluginName.FileExplorer);

That's unlikely going to work with NodeNext (Node16+). We should respect the encapsulation of the package

Fevol commented 3 months ago

Thanks for the quick response and many great pointers!

Please loot at my comment https://github.com/Fevol/obsidian-typings/pull/65#issuecomment-2147877581

I created a repo for you and me to identify all the migration pains.

Please create there an example where "moduleResolution": "bundler" is working and "moduleResolution": "NodeNext" doesn't. I will be more than happy to get it fixed.

Apologies, I should have clarified a bit more: it is not that NodeNext does not work for me, but that it requires an unnecessary amount of rewrites (imports breaking, having to add the extension everywhere for relative imports, etc.), that I gave up after trying for a hour. NodeNext did work for me in the test repository, but I got confused with something else probably.

This could require you to restart TypeScript server, but this usually happens only one time.

Yeah, that probably was the reason why it suddenly worked.

Having extensions is mandatory for NodeNext module resolution (to be precise, starting from Node16), so I strongly recommend we keep it that way for future compatibility. I already gave on the idea to keep .ts extensions and have to switch to the non-existing .js ones, but unfortunately we have to do it to support more client configurations.

That's absolutely fine for me then, making it as compatible as possible is the ideal situation, of course. I mainly suggested it because I didn't know that there are actually plugins that make use of NodeNext, I thought most were on ESNext or node. Thanks for pointing this out to me!

I will investigate if this is indeed required.

Since the types field is added, no changes should have to be made to already existing repositories. Specifying bundler and upgrading to TS5 is only necessary if you want to have /implementations import, AFAICS.

I disagree with your suggestion either. I think this is the way to go: https://github.com/mnaoumov/obsidian-typings-build-test/blob/master/project1/extensions.d.ts

I did try this, but sadly it did not work for me. I think either the declaration should be in its own file, or it is an issue with Esbuild, I haven't been able to confirm either way. The snippet above is the only one that worked for me.

Actually baseUrl SHOULD NOT be set, it is a bad confusing thing, because it breaks the relative paths and auto-complete. I would strongly advise to remove it instead.

You are right, my bad! I have the baseUrl specified for my commentator plugin, and I haphazardly copied it over. The path should be "./node_modules/@types/obsidian-typings/lib/implementations" however, otherwise you would get errors about: Non-relative paths are not allowed when 'baseUrl' is not set. Did you forget a leading './'?.

That's unlikely going to work with NodeNext (Node16+). We should respect the encapsulation of the package

Ah sorry, I saw it in https://github.com/mnaoumov/obsidian-typings-build-test/blob/master/automatic-type-extending/main.ts and assumed that it worked on your end. Yeah, method 1 is the way to go then.

I do wonder if it might be better to create a separate package instead for the /implementations, given all the hassle that it brings. I remember method 1 not working if you weren't using NodeNext or bundler.

mnaoumov commented 3 months ago

Thanks for the quick response and many great pointers!

Please loot at my comment #65 (comment)

I created a repo for you and me to identify all the migration pains.

Please create there an example where "moduleResolution": "bundler" is working and "moduleResolution": "NodeNext" doesn't. I will be more than happy to get it fixed.

Apologies, I should have clarified a bit more: it is not that NodeNext does not work for me, but that it requires an unnecessary amount of rewrites (imports breaking, having to add the extension everywhere for relative imports, etc.), that I gave up after trying for a hour. NodeNext did work for me in the test repository, but I got confused with something else probably.

In my opinion, rewriting to NodeNext is not unnecessary, it is getting rid of legacy code to make it more portable and future-proof.

But I realize we need to support all possible client configurations. So making sure it works in node and in bundler mode is the must. I am working on it.

This could require you to restart TypeScript server, but this usually happens only one time.

Yeah, that probably was the reason why it suddenly worked.

Having extensions is mandatory for NodeNext module resolution (to be precise, starting from Node16), so I strongly recommend we keep it that way for future compatibility. I already gave on the idea to keep .ts extensions and have to switch to the non-existing .js ones, but unfortunately we have to do it to support more client configurations.

That's absolutely fine for me then, making it as compatible as possible is the ideal situation, of course. I mainly suggested it because I didn't know that there are actually plugins that make use of NodeNext, I thought most were on ESNext or node. Thanks for pointing this out to me!

Well, at least all my plugins are on NodeNext. I didn't do the research to get the stats :)

I will investigate if this is indeed required.

Since the types field is added, no changes should have to be made to already existing repositories. Specifying bundler and upgrading to TS5 is only necessary if you want to have /implementations import, AFAICS.

Testing this theory now.

I disagree with your suggestion either. I think this is the way to go: https://github.com/mnaoumov/obsidian-typings-build-test/blob/master/project1/extensions.d.ts

I did try this, but sadly it did not work for me. I think either the declaration should be in its own file, or it is an issue with Esbuild, I haven't been able to confirm either way. The snippet above is the only one that worked for me.

Yes, the declaration has to be in its own .d.ts file. But please use our testing repository to provide an example that didn't work for you.

Actually baseUrl SHOULD NOT be set, it is a bad confusing thing, because it breaks the relative paths and auto-complete. I would strongly advise to remove it instead.

You are right, my bad! I have the baseUrl specified for my commentator plugin, and I haphazardly copied it over. The path should be "./node_modules/@types/obsidian-typings/lib/implementations" however, otherwise you would get errors about: Non-relative paths are not allowed when 'baseUrl' is not set. Did you forget a leading './'?.

Here is my bad, actually, I am the one who forgot the leading ./?

Fevol commented 3 months ago

Yes, the declaration has to be in its own .d.ts file. But please use our testing repository to provide an example that didn't work for you.

I pushed an example (copy of project1) where I showcase the issue. I misread the error in my original repository, it is not that the declaration file wasn't working properly, but it's the type of the plugin being wrong. My method (method 2) does work for this purpose.

(See: https://github.com/mnaoumov/obsidian-typings-build-test/tree/master/extension-test)

mnaoumov commented 3 months ago

@Fevol I have to apologize. My method was faulty and I didn't do enough testing

It's always a good idea to ad "skipLibCheck": false to actually test faulty .d.ts files

I did that in my repository and discovered the issue.

While technically your solution works, it adds the undesired confusion, because you declare the interface in obsidian module where it doesn't belong.

declare module "obsidian" {
  interface Plugins {
    plugins: Record<string, Plugin> & {
      commentator: CommentatorPlugin;
    }
  }
}

Unfortunately, we cannot just move the declaration to obsidian-typings module, because we will get a compile error.

Subsequent property declarations must have the same type.  Property 'plugins' must be of type 'Record<string, Plugin>', but here has type 'Record<string, Plugin> & { commentator: CommentatorPlugin; }'.ts(2717)

I came up with another more robust solution, which I am going to push in a bit

mnaoumov commented 3 months ago

@Fevol I came up with the following solution: https://github.com/mnaoumov/obsidian-typings-build-test/blob/master/extension-test/extensions.d.ts

import CommentatorPlugin from "./main.ts";

declare module "obsidian-typings" {
  interface PluginsPluginsRecord {
    commentator: CommentatorPlugin
  }
}

Here the intent is clear, we put it into obsidian-typings module and don't pollute into the obsidian module.

Unfortunately we couldn't just augment Record<string, Plugin> due to TS2717 compilation error.

So I came up with the solution where we replace Record<K,V> types with corresponding interfaces that we can now augment.

I extracted all record types from all places for consistency purposes, knowing that the most likely the most of them will never be augmented, but the consistency comes with its trade-offs.

mnaoumov commented 3 months ago

@Fevol I wrote build scripts to bundle typings and implementations into CommonJS format, so more clients can just use it.

Also added MIGRATION.md describing different gotchas.

Fevol commented 3 months ago

This is fantastic work, thank you so much! Tested this out on a new project, almost everything works out of the box. I only noticed one regression, aliasing /implementations does not seem to work anymore with the cjs format - or I am doing something completely wrong. This seems to not be a TS issue, but ESBuild, as far as I can understand (still investigating). Other than that, this PR (and all the others) are ready to merge for me.

Additional notes: it is a module resolution issue again (Could not resolve "obsidian-typings/implementations")

My current hypothesis is that this alias workaround would only work with NodeNext (does not work with node or bundler)

EDIT 1: In this SO thread, I found that typesVersions would be a legacy alternative for exports, but I haven't been able to get it to work still, or it is not relevant to the issue. (package.json)

{
  "typesVersions": {
    "*": {
      "implementations": [
        "./dist/implementations.d.ts"
      ]
    }
  }
}

mnaoumov commented 3 months ago

@Fevol did you read the README.md? There I show how to get implementations working for legacy scenario which is the case of obsidian-sample-plugin

Fevol commented 3 months ago

Yeah, I did try the methods listed in the README, neither worked with the bundler/legacy TS set-up. I'll try everything again to see if I made a mistake along the way.

Fevol commented 3 months ago

Tested everything again from scratch with the sample plugin, still getting a module resolution error for obsidian/implementations. Does the process work on your end, or do you notice a mistake below?

Specifically I did:

Clone sample plugin
Copy build of obsidian-typings containing only dist and package.json to @types/obsidian-typings

Update tsconfig.json with:

{
"compilerOptions": {
    "types": [
        "obsidian-typings"
    ],
    "paths": {
        "obsidian-typings/implementations": [
            "./node_modules/@types/obsidian-typings/dist/implementations.d.ts"
        ]
    }
    }
}

Add test lines for implementations import in main.ts


import { InternalPluginName } from "obsidian-typings/implementations";

console.log(InternalPluginName.FileExplorer)


5. Run `build` script of `package.json`

Fevol commented 3 months ago

Alright, I did manage to solve it by tweaking the ESBuild configuration, but I am not sure whether I like it.

// esbuild.config.mjs
const context = await esbuild.context({
    // ...
    alias: {
        "obsidian-typings/implementations": "./node_modules/@types/obsidian-typings/dist/implementations.cjs",
    },
    // ...
});

What do you think? Since implementations is not an essential part of the obsidian-typings package, I am fine with an admittedly ugly solution like this, otherwise I feel like we'd be stuck for a while trying to find another way to solve this.

mnaoumov commented 3 months ago

@Fevol

I made the following end-to-end scenario working

cd path/to/new/dir
git clone https://github.com/obsidianmd/obsidian-sample-plugin
cd obsidian-sample-plugin
npm install @types/obsidian-typings@path/to/obsidian-typings/root/with/compiled/dist --save-dev
# modify tsconfig.json
# modify main.ts
npm install
npm run build

tsconfig.json changes (I will update README.md for the changes required)

{
  "compilerOptions": {
    ...
    "paths": {
      "obsidian-typings/implementations": [
        "./node_modules/@types/obsidian-typings/dist/implementations.d.ts",
        "./node_modules/@types/obsidian-typings/dist/implementations.cjs"
      ]
  }
}

main.ts changes:

import { InternalPluginName } from "obsidian-typings/implementations";
console.log(InternalPluginName.FileExplorer);

Fevol commented 3 months ago

Oh that is much better, this works for me without issue. I really should've realized that the only difference with the previous version is that the .ts was compiled into two separate files, so obviously only aliasing one of them wouldn't work. 😅

~~Alright, I'll merge the PR's now (unless you still have something to add?). Then I'll update the README with this new correct version.~~

(Apologies, I did not read your full message, I'll wait till you've pushed the changes)

mnaoumov commented 3 months ago

@Fevol hold with merges. I want to try a better idea to solve the style-mod nightmare

mnaoumov commented 3 months ago

@Fevol honestly I don't know why are you still into @types/obsidian-typings approach. I removed the recommended label from this case in README.md (and put to the one that actually should be recommended)

Fevol commented 3 months ago

The main reason is that I use the set-up in my other plugins, so I know for sure that the new changes will also work on my (and other people's) repositories. Plus, I didn't know that installing under @types/ isn't necessary (anymore?) (#10, #13).

Either way, if it really has no use anymore, then I am fine with the approach being entirely removed from the Readme.

mnaoumov commented 3 months ago

@Fevol I made a final fix for style-mod nightmare which doesn't require anything from the library users. Now you can merge the PR. I will also go through all other PRs to ensure there are no merge conflicts

Fevol commented 3 months ago

Great! Thank you again for all your work on this massive PR.

Fevol / obsidian-typings