Rename FileSystem interface to CDVFileSystem

ajuanjojjj commented 9 months ago

Platforms affected

All (TypeScript definition changes)

Motivation and Context

Fixes the typescript conflict between the plugin and libdom's definitions Fixes #563 Closes #536

Description

Renamed the FileSystem interface to FileSystemCordova to resolve a conflict with LibDom's global interface

Testing

Tested against a company project, as well as the examples in the readme, using TSC

Checklist

[x] I've run the tests to see all new and existing tests pass
[ ] I added automated test coverage as appropriate for this change
[x] Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
[x] If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
[x] I've updated the documentation if necessary

peitschie commented 9 months ago

Unfortunately, a rename is not enough here. With this change in place, my test set (https://github.com/peitschie/cordova-plugin-file-type-tests) still sees this problem when your change here is applied.

index.ts:15:37 - error TS2345: Argument of type '(value: FileSystem | PromiseLike<FileSystem>) => void' is not assignable to parameter of type '(fileSystem: FileSystemCordova) => void'.
  Types of parameters 'value' and 'fileSystem' are incompatible.
    Type 'FileSystemCordova' is not assignable to type 'FileSystem | PromiseLike<FileSystem>'.
      Type 'FileSystemCordova' is not assignable to type 'FileSystem'.
        The types of 'root.createReader().readEntries' are incompatible between these types.
          Type '(successCallback: (entries: Entry[]) => void, errorCallback?: ((error: FileError) => void) | undefined) => void' is not assignable to type '(successCallback: FileSystemEntriesCallback, errorCallback?: ErrorCallback | undefined) => void'.
            Types of parameters 'successCallback' and 'successCallback' are incompatible.
              Types of parameters 'entries' and 'entries' are incompatible.
                Type 'Entry[]' is not assignable to type 'FileSystemEntry[]'.
                  Type 'Entry' is not assignable to type 'FileSystemEntry'.
                    Types of property 'getParent' are incompatible.
                      Type '(successCallback: (entry: Entry) => void, errorCallback?: ((error: FileError) => void) | undefined) => void' is not assignable to type '(successCallback?: FileSystemEntryCallback | undefined, errorCallback?: ErrorCallback | undefined) => void'.
                        Types of parameters 'successCallback' and 'successCallback' are incompatible.
                          Type 'FileSystemEntryCallback | undefined' is not assignable to type '(entry: Entry) => void'.
                            Type 'undefined' is not assignable to type '(entry: Entry) => void'.

Typescript's conflict arises because Window.requestFileSystem is defined by libdom as well as this plugin. Typescript doesn't use the interface names to match types, but rather pays attention to the interfaces being compatible with each other.

The only solution is either https://github.com/apache/cordova-plugin-file/pull/536 (which lies about cordova's own types to maintain compatibility) or this plugin should move off the requestFileSystem method to a new method that does not conflict with libdom.

ajuanjojjj commented 9 months ago

I don't think the change is at fault here, but rather the test; index.ts:15:37 points out that the promise's type is FileSystem (As in, libdom's type) instead of FileSystemCordova (As in, the one from the plugin)

Simply changing the promise's type to FileSystemCordova, using a "Promisify" lib, or using the Utility Types from TS to create this monstrosity <Parameters<Parameters<Window["requestFileSystem"]>[2]>[0]> makes the tests compile successfully

ajuanjojjj commented 9 months ago

Either way, would it be possible to add your tests to the main cordova-plugin-file repo? TypeScript 4.4 came out the 2021/08/26, and the first report of the issue seems to be your PR on the 2022/08/17, a full year later, and it seems to have garnered little to no attention for the two years it's been an issue.

Having both the error pointed out when passing tests, and a way of easily checking if a fix addresses them, would probably help if this happens again in the future

peitschie commented 9 months ago

Ah! You're absolutely right @ajuanjojjj

In that case, I agree your fix here is the right one.

ajuanjojjj commented 9 months ago

Amended the commit so it aligns with contribution guidelines, so I think the PR is ready for review. Should I ping someone in particular?

breautek commented 9 months ago

I think this would be considered a breaking change for anybody using TypeScript with this plugin, thus merging will have to wait until Apache starts preparing a major release.

Unfortunately, Apache just made a major release not that long ago, so it may be some time before another major release is prepared.

breautek commented 9 months ago

Either way, would it be possible to add your tests to the main cordova-plugin-file repo?

I'm also not sure if this would be accepted... I'm a TS user myself but Apache Cordova is not. Actually testing the TS would mean requiring typescript packages to run the test.

Personally, if Apache wants to keep maintaining the .d.ts files, then I see myself +1 a PR that adds typescript dev dependency that tests "compiling" a sample .ts file with noEmit. Otherwise Apache should forgo the responsibility of maintaining the type definitions and leave it to the TS community like DefinitelyTyped, who can probably have a faster release cycle than Apache can to resolve issues such as this one.

Just a thought.

ajuanjojjj commented 9 months ago

I think this would be considered a breaking change for anybody using TypeScript with this plugin, thus merging will have to wait until Apache starts preparing a major release...

I really think its the complete opposite; we currently have a broken TS package, that errors out with a fresh install, and that can't be used unless manually fixing it with patch-package or casting it/@ts-ignore'ing it. If anything the update should be expedited, not delayed

breautek commented 9 months ago

I think this would be considered a breaking change for anybody using TypeScript with this plugin, thus merging will have to wait until Apache starts preparing a major release...

I really think its the complete opposite; we currently have a broken TS package, that errors out with a fresh install, and that can't be used unless manually fixing it with patch-package or casting it/@ts-ignore'ing it. If anything the update should be expedited, not delayed

It's my understanding based on the original report in https://github.com/apache/cordova-plugin-file/issues/563 that the issue only affects newer typescript/types versions. So while modern typescript releases may not work properly, anybody still using an older typescript release may still be using the current types as is fine, and they wouldn't expect the types to be renamed in a minor or patch release. This is why it still needs to be treated as a major update.

ajuanjojjj commented 9 months ago

I think this would be considered a breaking change for anybody using TypeScript with this plugin, thus merging will have to wait until Apache starts preparing a major release...

I really think its the complete opposite; we currently have a broken TS package, that errors out with a fresh install, and that can't be used unless manually fixing it with patch-package or casting it/@ts-ignore'ing it. If anything the update should be expedited, not delayed

It's my understanding based on the original report in #563 that the issue only affects newer typescript/types versions. So while modern typescript releases may not work properly, anybody still using an older typescript release may still be using the current types as is fine, and they wouldn't expect the types to be renamed in a minor or patch release. This is why it still needs to be treated as a major update.

Although its true that older versions did not have this issue, said versions are almost over two years old at this point. Still, if this is considered an issue, typesVersions could be used to also declare FileSystem on <4.4 versions of TypeScript, even if I personally think that most TS users will have instead patched the plugin themselves.

Regarding the suggestion to stop maintaining d.ts files for the plugin, I wouldn't mind at all if d.ts files would be moved to DefinitivelyTyped, but I should note that the complete opposite was done 6 years ago, deprecating the DefinetivelyTyped package in favour of shipping the types with the plugin.

ajuanjojjj commented 9 months ago

About the test stuff; adding tests to the package, if done properly, would help to both keep the d.ts files up to date, and help contributing users by providing some more assurances that their changes are ok, since, at the moment, the only tests are a simple eslint pass. You already need to install eslint globally to run said tests (called without npx, missing from devDependencies) and also run npm install to get "@cordova/eslint-config", so I don't think also adding typescript to the dependencies (and probably eslint if we're at it) is that tough of an ask. Still, it might be better to discuss it on a different issue, since although its related to the PR, its a considerably different thing.

breautek commented 9 months ago

Regarding the suggestion to stop maintaining d.ts files for the plugin, I wouldn't mind at all if d.ts files would be moved to DefinitivelyTyped, but I should note that the complete opposite was done 6 years ago, deprecating the DefinetivelyTyped package in favour of shipping the types with the plugin.

It is a struggle. 6 years ago there was a lot more maintainers actively working on Cordova, including folks from Microsoft. Today there's only a handful of active maintainers and we've been downsizing quite a bit already (e.g. deprecating OSX/Windows platforms, in favour of electron that can support both easier). I'm not 100% sure but I think most current maintainers don't use TS themselves. I know I don't really think of TS types myself when contributing to Cordova... cause normally I just let TSC compile the types definitions. Chances are, most of our typedefs are probably not even reflective of the actual interface across the Cordova codebase.

About the test stuff; adding tests to the package, if done properly, would help to both keep the d.ts files up to date, and help contributing users by providing some more assurances that their changes are ok, since, at the moment, the only tests are a simple eslint pass.

There's actually more test, just Cordova has a specialized tool to run them, it's not ran through npm test. But adding interface tests isn't hard. The concern is adding a new and rather large dependency (even if it's just a devDependency) in which needs to be maintained for audit tests. But like I said, personally I do think it's necessary if we continue maintaining the typescript definition files.

I'll seed the setup for that, that at least loads in the d.ts file to ensure runs on TS 5.1. FWIW I observed the type conflicts, and observed your PR addresses it. Removing the d.ts files I think will require a formal cordova vote on our mailing list and until that happens, I'll treat the typedefs as something still maintained.

ajuanjojjj commented 9 months ago

Updated title, interface, and commit description to align with iOS naming conventions, changing FileSystemCordova to CVDFileSystem

apache / cordova-plugin-file