[Windows] readDir() .isFile() and .isDirectory() don't resolve correctly

[AchillesChristianPrinz]

In the readDir method the resulting callbacks isFile() and isDirectory() compare the wrong types.
On the JS side you assume that file.type is a string when comparing it to any of those values: const { FileTypeDirectory, FileTypeRegular } = RNFS.getConstants()

react-native-fs/src/index.ts

Line 64 in 5df8019

isFile: () => file.type === FileTypeRegular,

react-native-fs/src/index.ts

Line 65 in 5df8019

isDirectory: () => file.type === FileTypeDirectory,

But at least on react-native-windows and probably also on android the type of file.type is a number. This results in both callbacks always resolving to false wheter it is a file or a directory.

Here are the assignments on the native side:

react-native-fs/android/src/main/java/com/drpogodin/reactnativefs/ReactNativeFsModule.kt

Line 529 in 5df8019

fileMap.putInt("type", if (childFile.isDirectory) 1 else 0)

react-native-fs/windows/ReactNativeFs/ReactNativeModule.cpp

Line 511 in 5df8019

itemInfo["type"] = item.IsOfType(StorageItemTypes::Folder) ? 1 : 0;

My temporary solution (just tested on rn-windows) is to cast the type to a String:

isFile: () => String(file.type) === FileTypeRegular,
isDirectory: () => String(file.type) === FileTypeDirectory

But the whole thing should probably be deprecated and the type: 'file' | 'directory' be passed through from the native side, instead of using two callbacks to resolve a possible number or string to a boolean

[birdofpreyru]

Thanks @AchillesChristianPrinz !

Looking at the readDir() test in Example App, where isDirectory() and isFile() checks are disabled for Windows, I believe it was broken on Windows only. I see, on Android the implementation violates the typing at TypeScript side, but works correctly in the runtime, because both the field, and the corresponding constants are assigned numeric values rather than strings.

But the whole thing should probably be deprecated and the type: 'file' | 'directory' be passed through from the native side, instead of using two callbacks to resolve a possible number or string to a boolean

I agree. The current goofy design is inherited from the original upstream code. I thought about this exact change, among other stuff, for v3 of the library (to keep the current v2 as close to the drop-in replacement of the original library, as possible); but v3 is unlikely to happen any time soon.

So, for now the fix will be to ensure correct string values are assigned to related fields and constants on the native side on Windows and Android; I'll do it when updating Windows version to [email protected] (which is still on hold, because #76).

[AchillesChristianPrinz]

Thanks for your quick response. This won't probably last long since the are already @ preview5 or so.

[alessandro-bottamedi]

I have the same problem on iOS (RN 0.76.1), isFile and isDirectory resolve always "false".

[birdofpreyru]

@alessandro-bottamedi it might be different. On iOS there are more resource types, beyond directory and regular file: https://developer.apple.com/documentation/foundation/nsurlfileresourcetype. As of now (the same as in the original upstream), isFile() evaluates true just for NSURLFileResourceTypeRegular, and isDirectory() evals true just for NSURLFileResourceTypeDirectory.

[alessandro-bottamedi]

@alessandro-bottamedi it might be different. On iOS there are more resource types, beyond directory and regular file: https://developer.apple.com/documentation/foundation/nsurlfileresourcetype. As of now (the same as in the original upstream), isFile() evaluates true just for NSURLFileResourceTypeRegular, and isDirectory() evals true just for NSURLFileResourceTypeDirectory.

But with RN 0.75 and version 2.28 of the package, isFile and isDirectory respond correctly with the same files…

[birdofpreyru]

Update

It turns out, making this library and its Example App to work with [email protected] (New Arch) on Windows is an undertaking too large for me, unless it is sponsored by somebody (#76 (comment)). For similar reasons I can't support multiple versions of RN / fix something in older versions of the library. Thus, the Windows part of the issue will be put On Hold indefinitely.

At the same time, I found & fixed the issue for iOS / macOS (it turned out to be a regression introduced when implementing support of security-scoped URLs). I'll probably release it over the weekend, or the next week, once complete some more fixes related to special characters support in path names (#82, #85) on Android / iOS / macOS.

[pcprinz]

Maybe I'll take the time to make a PR for the tasks on the windows side, once you released the changes. I just have to figure out how to build and test the project.

But the other way around, I can build and test Windows and Android only, so if I have to make changes on the js side you'll probably have to make regression tests for iOS/MacOS.

[birdofpreyru]

how to build and test the project.

Yeah... that's the crux of the problem — Example App depends on react-native-static-server, and that, with its own Example App, depends both on react-native-fs and react-native-webview; to make everything build with [email protected] / New Arch on Windows, it requires upgrading all three libs at the same time, and react-native-webview in particular is a problem, as it isn't well aligned with the template code created by react-native-windows, thus I can't easily upgrade that without digging into implementation details.

if I have to make changes on the js side

I believe, the fix here should be on native side — just correcting the values assigned to related fields and constants to ensure they are all strings. Quite simple, but a pain to test without being able to build Example App.

[pcprinz]

I believe, the fix here should be on native side — just correcting the values assigned to related fields and constants to ensure they are all strings. Quite simple, but a pain to test without being able to build Example App.

You're totally right - This is all on the native side. I already got this patched in my app.

I got the example app running on windows, but only 10/25 tests passed. 2 of the failing were missing an await. Me and the copilot are currently into a refactoring of the tests. I'll try to get everything passed for windows. Afterwards It would be nice if you could run the tests for the other platform to ensure everything's still fine.

When this is done I'll fix the "Umlauts" and isDirectory / isFile issues, as well as the "new NativeEmitter" warning on windows.

... to make everything build with [email protected] / New Arch on Windows ...

I'm doing this on the uwp Version 0.76 that you installed for the example app. As it seems it's still possible to do it "the old way".
Imo It's better to get the issues done before upgrading than just waiting for the dependency-deadlock to disappear - has to be done anyways.

Maybe there has to be a way for the example app to release the dependency on react-native-static-server - would make things way easier.