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

Make types compatible with Typescript 4.4 #500

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fcamblor
Copy link

@fcamblor fcamblor commented Sep 27, 2021

Platforms affected

None, Typescript type definition only

Motivation and Context

Typescript 4.4 has introduced a lot of changes in lib.dom.d.ts file, which raises a lot of compilation errors with the plugin.
These changes makes it compatible with it.

Description

  • Entry renamed to FileSystemEntry
  • FileEntry renamed to FileSystemFileEntry
  • DirectoryEntry renamed to FileSystemDirectoryEntry
  • Commented FileSystem interface which already exists into lib.dom.d.ts
  • Commented field duplications for those interfaces, to avoid error TS2687
     error TS2687: All declarations of 'xxx' must have identical modifiers.

Most of the changes are naming changes to follow Typescript renames.

However, this may require some changes on the application side if :

  • the old Type names were referenced in your code (migrate them to the new name, or make type alias with the old name if there are too much occurences of it in your code)
  • If you are on a TS version older than 4.4, you may miss the commented parts if you were relying on it in your code. In that case, I'd suggest to declare following interfaces somewhere in your code :
declare interface FileSystem {
    /* The name of the file system, unique across the list of exposed file systems. */
    name: string;
    /** The root directory of the file system. */
    root: FileSystemDirectoryEntry;
}

declare interface FileSystemEntry {
    /** FileSystemEntry is a file. */
    isFile: boolean;
    /** FileSystemEntry is a directory. */
    isDirectory: boolean;
    /** The name of the entry, excluding the path leading to it. */
    name: string;
    /** The full absolute path from the root to the entry. */
    fullPath: string;
    /** The file system on which the entry resides. */
    filesystem: FileSystem;
}

⚠️ Important notes :

  • I updated doc/plugins.md file to reflect the naming changes, but I'm unsure of the pertinence of it given that we're going to have discrepancies (in namings) between native code's semantics and javascript's semantic. I can rollback these change if you prefer.
  • Due to the fact that these type definitions are incompatible with TS 4.3 (and before), and TS 4.4 has been released only recently (end of Aug 2021), we may consider providing a dedicated index.ts44.d.ts and tell people to reference it instead of the default index.d.ts
  • Some care should be added to the README's Upgrading notes section to pinpoint these changes (I didn't added any entry as I don't know which version of the plugin may be concerned by this change)

Testing

There wasn't any TS-based tests in place currently, didn't spent time to introduce it as I'd consider it as a whole dedicated topic.

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

bcalvez added a commit to bcalvez/cordova-plugin-file that referenced this pull request May 6, 2022
Support Typescript 4.4 (apache#500)
@peitschie
Copy link
Contributor

To maintain backwards compatibility with existing type definitions, we could add the following type aliases to the index.d.ts here:

type Entry = FileSystemEntry;
type DirectoryEntry = FileSystemDirectoryEntry;
type FileEntry = FileSystemFileEntry;

Other than that, this works well for me. @breautek any chance we might merge this?

@fcamblor
Copy link
Author

fcamblor commented Aug 16, 2022

As mentioned in my initial post, I'm not sure if putting these declarations into index.d.ts is viable (don't forget that FileSystemEntry doesn't exist on TS < 4.4)

I think we can't do better than providing a specific index.ts44.d.ts file for this purpose, and ask people to reference it once they migrate to TS 4.4+

@peitschie
Copy link
Contributor

@fcamblor yep, that makes sense! I missed that, sorry! Typescript 4.4 was first released a bit over a year ago, so I guess at some point people will be more likely to use 4.4+ and hit this issue. Not really any easy answer, is there 😞

One trivial tweak here if anyone runs into this is to add "skipLibCheck": true to the tsconfig.json

Seems that "breakages in node_modules and the JS standard libraries which you do not want to deal with" is a literal use case according to https://www.typescriptlang.org/tsconfig#skipLibCheck.

I'm having a quick fiddle to see if there's a way to have our cake and eat it though.

peitschie added a commit to peitschie/cordova-plugin-file-type-tests that referenced this pull request Aug 17, 2022
@peitschie
Copy link
Contributor

I've raised a slightly different pr at #536 which rebalances the compatibility trade-off a bit differently to yours here @fcamblor .

It's slightly less-nice than master in terms of type accuracy, but this works happily with newer versions of TypeScript, so perhaps might still be worth considering?

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.

2 participants