ajuanjojjj commented on PR #588: URL: https://github.com/apache/cordova-plugin-file/pull/588#issuecomment-1656137629
> > > 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](https://github.com/microsoft/TypeScript/releases/tag/v4.3.5). Still, if this is considered an issue, [typesVersions](https://github.com/microsoft/TypeScript/pull/26568) 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. About the tests; I wouldn't mind at all if d.ts files would be moved to DefinitivelyTyped, but I should note that the [complete](https://www.npmjs.com/package/@types/cordova-plugin-file?activeTab=versions) [opposite](https://github.com/apache/cordova-plugin-file/commit/3abbdee693862b4509f5767ea9bad83259845aed) was done 6 years ago, deprecating the DefinetivelyTyped package in favour of shipping the types with the plugin. Instead, 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
