Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(types): Adjust types to fix incompatibilities with TypeScript 4.4+ #536

Closed
wants to merge 1 commit into from

Conversation

peitschie
Copy link
Contributor

@peitschie peitschie commented Aug 17, 2022

Platforms affected

Typescript type definitions only. No platform impacts.

Motivation and Context

Changes in libdom by the TypeScript project led to conflicting definitions with this plugin. This is an attempt following on from #500 to try and address the compatibility problem.

Description

The key point that joins libdom and this plugin together are via the root property
on the FileSystem interface. This results in the DirectoryEntry and Entry types needing
to align with libdom's FileSystemDirectoryEntry & FileSystemEntry.

This introduces 3 slight irregularities in order to be compatible with libdom

  1. getParent successCallback needs to be optional
  2. getParent errorCallback needs to be typed as a DOMException, despite it being a FileError in reality
  3. getFile and getDirectory success types need to be relaxed to the base Entry

Testing

I've created a test repository with several typescript versions, and ensured each of these compiles without issue: https://github.com/peitschie/cordova-plugin-file-type-tests

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

Changes in libdom by the TypeScript project led to conflicting definitions with this plugin.

The key point that joins libdom and this plugin together are via the `root` property
on the FileSystem interface. This results in the DirectoryEntry and Entry types needing
to align with libdom's FileSystemDirectoryEntry & FileSystemEntry.

This introduces 3 slight irregularities in order to be compatible with libdom

1. getParent successCallback needs to be optional
2. getParent errorCallback needs to be typed as a DOMException, despite it being a FileError in reality
3. getFile and getDirectory success types need to be relaxed to the base Entry
@cadesalaberry
Copy link

How can we help on this PR to get it merged?

@cadesalaberry
Copy link

@erisu can we help?

@erisu
Copy link
Member

erisu commented Feb 6, 2023

I dont use TypeScript much to be able to confirm the changes but see if someone can take a look and review.

From my own understanding, these changes do not appear to be correct, but can you explain to me why they would be considered correct?

For example.

Before PR:

    getParent(successCallback: (entry: Entry) => void,
        errorCallback?: (error: FileError) => void): void;

After PR:

    getParent(successCallback?: (entry: Entry) => void,
        errorCallback?: (error: DOMException) => void): void;

This is telling users that the error's callback error property is DOMException but when looking at the actual code, this is not true.

The getParent method in the Entry.js file, is clearly passing the FileError

FileError.js appears to have added extra APIs for the File API specification. (which might have been added long time ago during the drafting of the specification.)

Same with the getFile and getDirectory method. It changes the success callback to Entry but in code, respectively, it returns FileEntry and DirectoryEntry.

@peitschie
Copy link
Contributor Author

From my own understanding, these changes do not appear to be correct, but can you explain to me why they would be considered correct?

Your understanding here is correct, in that in order to remain compatible with the libdom interface, our only option is to make our own types inaccurate 😞

I haven't really found any alternative solution, as we sit on top of the global interface which libdom expects to control.

The only way that I can think of to keep fully accurate types would be to expose this under an alias instead so we don't clash with the types.

It's an unfortunate situation!

@peitschie peitschie closed this Jul 28, 2023
ajuanjojjj added a commit to ajuanjojjj/cordova-plugin-file that referenced this pull request Jul 28, 2023
- Rename FleSystem interface to FileSystemCordova
ajuanjojjj added a commit to ajuanjojjj/cordova-plugin-file that referenced this pull request Aug 1, 2023
- Rename FleSystem interface to FileSystemCordova
ajuanjojjj added a commit to ajuanjojjj/cordova-plugin-file that referenced this pull request Aug 1, 2023
- Rename FleSystem interface to CVDFileSystem
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants