Rename FileSystem interface to CDVFileSystem

[ajuanjojjj]

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

• I've run the tests to see all new and existing tests pass
• I added automated test coverage as appropriate for this change
• Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
• 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)
• I've updated the documentation if necessary

[peitschie]

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 #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]

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]

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]

Ah! You're absolutely right @ajuanjojjj

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

[ajuanjojjj]

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]

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]

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]

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]

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.

[ajuanjojjj]

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]

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]

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]

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

[breautek]

Thank you.

I think everything looks good, however I still think I have to wait until we are ready to prepare a new major release before I can merge.

For those who require the change immediately, the plugin can be forked and this patch can be applied

git remote add ajuanjojjj https://github.com/ajuanjojjj/cordova-plugin-file.git
git fetch ajuanjojjj
git checkout rel/8.0.0
git checkout -b cdvfilesystem
git cherry-pick e4cf9441e9d692940936f0e98c93fa9d73dff80f
git push

Then you can install the fork:

cordova plugin remove cordova-plugin-file
cordova plugin add <your-github-fork-url>

It would be recommended that when applying the patch, to checkout and branch of the rel/8.0.0 before doing the cherry-pick so that you're applying the patch on top of a voted release.

Alternatively ajuanjojjj's repo can be used directly:

cordova plugin remove cordova-plugin-file
cordova plugin add https://github.com/ajuanjojjj/cordova-plugin-file.git#e4cf9441e9d692940936f0e98c93fa9d73dff80f

But personally I always like to maintain my own version in my projects rather than having a direct dependency to someone's repository which may change in the future.