New Implementation of RNFS for Windows

[avmoroz]

trafficstars

Created new implementation of RNFS for Windows using C++. Previous implementation is located in the windows_legacy folder.

copyFolder implemented to account for slow file transfers.

[exotexot]

Amazing! :)

[itinance]

Thank you very much! How should we proceed here? I'd like to have at least one or two other devs testing this PR before merging. However, I don't have any Windows machine. So i definitely need support here :)

[stmoy]

@itinance - I'll poke the RN folks at Microsoft to get some more eyes on this!

[exotexot]

Using this PR for about a month now. Using downloadFile and uploadFile including authorisation Headers, stat, readDir, writeFile, and read and copyFile across all platforms, iOS, android, macOS (catalyst) and Windows and it’s working as expected! Great job!

[emmanuj]

Noob question here but would this work with auto-linking for windows out of the box? And is it possible to use the API directly in C# without any additional configuration?

[creambyemute]

@avmoroz This is 🔥! Gonna try it tomorrow, I'm atm re-initialiasing RN-Windows for our App and not much is missing, Kudos.

[itinance]

@asklar thank you very much for the review! As I can see, @avmoroz did many changes according to your appreciated comments. How can we proceed here?

[asklar]

Thanks @avmoroz and @itinance I'll take another look in the next couple of days/early next week

[creambyemute]

@itinance Question about the .exists method on android/ios: Does it behave the same on iOS/Android when passing (the only difference is the missing / at the end):

• startPath/somePath/
• startPath/somePath

I'm asking because I had to modify my .exists calls for folders for RN-Windows (@asklar @avmoroz ): (Path seperators are set correctly)

if (isWindows) {
    const directoryExists = await this.RNFS.exists(targetDirectory.slice(0, -1)); // Remove the trailing slash, otherwise it woudl always yield false even if the directory exists
    if (!directoryExists) {
        await this.mkdir(targetDirectory.slice(0, -1)); // Remove the trailing slash, otherwise it would fail
    }
    await this.RNFS.writeFile(target, content, encoding);
} else {
    // Use RN-Fetch-Blob for directly writing
    this.RNFetchBlob.fs.writeFile(...)
}

I'm now testing the RNFS RN-Windows implementation after I sorted out the RN-Gesture-Handler + React-Navigation problems. Should be able to reach the Dashboard tomorrow and test some more RNFS functionalities.

Edit: Ah and btw, the autolinking worked for me, didn't have to do anything for RN-FS to work.

[avmoroz]

Hi @creambyemute , I modified the .exists method in the Windows implementation to account for one trailing slash at the end of the path.

I checked the Android implementation, and found that you can have an almost unlimited number of trailing slashes after an existing file and it will be registered. I felt that one trailing forward slash or backslash was reasonable, and more than that was unnecessary. I could not check the iOS implementation because I do not have the necessary devices.

Please let me know if I ought to account for more than one trailing slash.

Thank you for your feedback.

[creambyemute]

Hey @avmoroz I honestly think one should be enough for the start. If more trailing slashes are passed it's often somewhat of a user/developer error?

I have just tested it and it seems to work fine without the above workaround on .exists.

Next question: .mkdir also fails without removing the trailing slash, I have looked at the code and just saw the reference to Apples createDirectoryAtPath method which seems fine to me. But does createDirectoryAtPath on osx/ios also fail on a trailing slash? Android is probably going to successfully create the directory?

Thanks a lot for your effort so far!

@itinance I should soon be able to test more features like copyFile, moveFile and downloads for some more in use review 👍

If anyone is interested, I have made some changes to redux-persist-fs-storage to be Windows compatible: https://github.com/wwimmo/redux-persist-fs-storage

[avmoroz]

Hey @creambyemute ,

I also checked and .mkdir to behaves the same way on Android. I've made a commit to account for this, and hopefully the behavior is consistent across iOS and Android.

I'm looking forward to your feedback on the other features!

[creambyemute]

Hey @avmoroz,

I have tested RN-FS + Redux-Persist-FS-Storage with Windows and the trailing slash didn't seem to bother anymore. I'll test iOS as well (through redux-persist-fs-storage) soon after resolving the build error I have there since updating RN-Config.

I guess tomorrow or on monday/tuesday I'll come around to replace the downloading & uploading files method stubs which would additionally include copyFile, moveFile & unlink.

[creambyemute]

Hey @avmoroz

So I got around to test some more functionalities like downloadFile, copyFile, moveFile and unlink and they seemed to work fine for me.

I've found one more odd behaviour: .appendFile on iOS and Android seems to create the file if it doesn't exist, while the CPP implementation doesn't.

I've had to workaround it for windows by first checking&creating the targetDirectory and then also checking&creating the file.

async function appendFile(
        filepath: string,
        targetDirectory: string,
        contents: string,
        encodingOrOptions?: string
    ): Promise<void> 
{
        if (Platform.OS === "windows") {
            const doesTargetDirectoryExist = await RNFS.exists(targetDirectory);
            if (!doesTargetDirectoryExist) {
                await RNFS.mkdir(targetDirectory);
            }
            const doesFileExist = await RNFS.exists(filepath);
            if (!doesFileExist) {
                await RNFS.writeFile(filepath, "", encodingOrOptions);
            }
        }
        return await this.RNFS.appendFile(filepath, contents, encodingOrOptions);
}

I'm also getting an Error on writeFile at that point (though it still works as expected), but that's only happening on Login. Probably because of multiple/simultaneuos not awaited logging calls in my app. Doesn't happen after the login anymore (because the file is created already at that point). This error does not appear on iOS/Android.

Error: Invalid pointer
    at Object.promiseMethodWrapper [as writeFile] (VM4 index.bundle:2386)
    at Object.writeFile

[avmoroz]

Hey @creambyemute,

I modified appendFile() to account for the inconsistency between this implementation and the Android and iOS implementations. Please let me know if this error also comes up with this implementation.

Sorry it took a while to get back to you.

[creambyemute]

Hey @avmoroz No worries for the delay. I'll test the change soon, fixing up some other mvp features in the app for windows right now.

Will let you know after christmas.

Have some good holidays 👍

[exotexot]

Hello everyone and happy new year.

Any updates on this. Really want to see this merged :)

[stmoy]

Happy new year to you too, @exotexot!

@asklar - do you sign off on this?

@itinance - once asklar signs off, is this good to be merged or are there outstanding things to address?

[creambyemute]

Hey @avmoroz and @stmoy

I have tested a bit more and found something weird when I was trying out the windows implementation of react-native-sketch-canvas and tried to move the picture to the app folder:

If I call .exists(...) with the Picture Library folder (Drive:\Users\{user}\Pictures\) or a file path within it (Drive:\Users\{user}\Pictures\RNSketchCanvas\somePicture.jpg) RN-FS crashes the app. I wasn't able to get an error logged.

read, copy and moveFile do work on the Pictures Library folder though.

See last comment on https://github.com/terrylinla/react-native-sketch-canvas/pull/182

[jaimecbernardo]

Checking the implementation, it looks like read, copy and moveFile are using winrt::Windows::Storage implementations while .exists relies on std::filesystem.

Is it possible that std::filesystem APIs don't work correctly on paths outside the Application's sandbox, such as the Pictures Library and winrt::Windows::Storage would have to be used to take into account manifest capabilities checks, if the file was previously picked by a FilePicker, etc...?

[stmoy]

I'll let @avmoroz chime in, but my hunch is that @jaimecbernardo is correct - since RNW builds a sandboxed UWP app, I'd imagine that std::filesystem wouldn't work - but I'm just guessing.

[asklar]

yes, std::filesystem APIs that touch the filesystem will crash the app

[avmoroz]

Hey everybody,

Sorry for the late response.

@creambyemute , I just pushed a commit to address this particular issue. I ran my unit tests and tried it out with my demo app, but please let me know if this implementation of exists() does not resolve your issue or raises new ones for you.

As @jaimecbernardo , @stmoy , and @asklar have pointed out, using std::filesystem to touch files and folders outside the sandbox will cause a crash. I looked through my implementations, and found that appendFile(), mkdir(), stat(), and unlink() have the potential to do that (they utilize std::filesystem::create_directories(), std::filesystem::is_directory(), and std::filesystem::exists). I'll try have new implementations without those std::filesystem functions within the next two days.

[avmoroz]

Hey everybody,

I removed all references to std::filesystem::create_directories(), std::filesystem::is_directory(), and std::filesystem::exists() were removed from appendFile(), mkdir(), stat(), and unlink(). I ran some unit tests and manual tests, and it seems like all the functionality is retained.

If there are any issues, concerns, or comments, I'm happy to hear them out and make modifications as necessary

[creambyemute]

@avmoroz I've got around to test the new implementations. Checking if the signature from rn-sketch-canvas exists works now. Copying/moving worked as well.

Unfortunately, directory creation seems to be working quite different (regression?) now than before:

• I have to call mkdir (make sure directory exists) before doing any copy/move actions, otherwise it complains
• If I call mkdir on an already existing directory, it complains, so you have to add a !exist check first.

I think Android handles this gracefully. Should one of those behaviours be expected or should we have to adapt our JS implementations to check for existence of directories before mkdir/copyfile/movefile etc?

Sorry for being so picky/persistent!

[avmoroz]

Hey @creambyemute ,

I did some investigation, and your issue with directory creation was an error on my part. mkdir() is not supposed to provide a promise rejection if the directory in question already exists or was not able to be created. It no longer provides a rejection as of this most recent commit.

I also dug around and realized that my implementation of mkdir() did not create parent directories if of the new directory if they did not initially exist. Hopefully that hasn't been an issue for you, but that has also been fixed in the most recent commit.

Just to clarify: was your issue with mkdir() specifically, or were you having issues with copyfile() and movefile() as well?

[creambyemute]

@avmoroz I noticed it while trying to moveFile/copyFile to a not yet existing directory. I then added mkdir calls and noticed that it then complained about the directory existing already (on a second run).

Despite that, moveFile/copyFile seemed to work fine

[exotexot]

Can we talk about what the next steps might be in this regard?

[stmoy]

Ping @avmoroz - looks like there is a merge conflict now. Can you please resolve and identify if there is any outstanding work? If there isn't any more work, is this good to get merged?

[creambyemute]

I've tried upgrading our Project to 0.64 and it fails building on react-native-fs. I then tried it with a fresh 0.64 init project and added rn-fs in there and it fails building as well on react-native-fs.

I create an issue in the rn-w repository first, but I think it rather belongs here after some more investigation: https://github.com/microsoft/react-native-windows/issues/7585

The npx react-native run-windows output I get is:

5>Done Building Project "C:\Users\thoma\dev\react\RN064Init2\node_modules\@terrylinla\react-native-sketch-canvas\windows\RNSketchCanvas\RNSketchCanvas.vcxproj" (default targets).
    11>C:\Users\thoma\dev\react\RN064Init2\node_modules\react-native-windows\Microsoft.ReactNative.Cxx\JSI\JsiApiContext.cpp(9,40): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int [C:\Users\thoma\dev\react\RN064Init2\node_modules\react-native-fs\windows\RNFS\RNFS.vcxproj]
    11>C:\Users\thoma\dev\react\RN064Init2\node_modules\react-native-windows\Microsoft.ReactNative.Cxx\JSI\JsiApiContext.cpp(9,29): error C2146: syntax error: missing ';' before identifier '__ImageBase' [C:\Users\thoma\dev\react\RN064Init2\node_modules\react-native-fs\windows\RNFS\RNFS.vcxproj]
    11>C:\Users\thoma\dev\react\RN064Init2\node_modules\react-native-windows\Microsoft.ReactNative.Cxx\JSI\JsiApiContext.cpp(34,5): error C2065: 'HMODULE': undeclared identifier [C:\Users\thoma\dev\react\RN064Init2\node_modules\react-native-fs\windows\RNFS\RNFS.vcxproj]
    11>C:\Users\thoma\dev\react\RN064Init2\node_modules\react-native-windows\Microsoft.ReactNative.Cxx\JSI\JsiApiContext.cpp(34,13): error C2146: syntax error: missing ';' before identifier 'currentDllHanlde' [C:\Users\thoma\dev\react\RN064Init2\node_modules\react-native-fs\windows\RNFS\RNFS.vcxproj]
    11>C:\Users\thoma\dev\react\RN064Init2\node_modules\react-native-windows\Microsoft.ReactNative.Cxx\JSI\JsiApiContext.cpp(34,13): error C2065: 'currentDllHanlde': undeclared identifier [C:\Users\thoma\dev\react\RN064Init2\node_modules\react-native-fs\windows\RNFS\RNFS.vcxproj]
    11>C:\Users\thoma\dev\react\RN064Init2\node_modules\react-native-windows\Microsoft.ReactNative.Cxx\JSI\JsiApiContext.cpp(34,49): error C2061: syntax error: identifier 'HMODULE' [C:\Users\thoma\dev\react\RN064Init2\node_modules\react-native-fs\windows\RNFS\RNFS.vcxproj]
    11>C:\Users\thoma\dev\react\RN064Init2\node_modules\react-native-windows\Microsoft.ReactNative.Cxx\JSI\JsiApiContext.cpp(35,101): error C2065: 'currentDllHanlde': undeclared identifier [C:\Users\thoma\dev\react\RN064Init2\node_modules\react-native-fs\windows\RNFS\RNFS.vcxproj]
    11>C:\Program Files (x86)\Windows Kits\10\bin\10.0.18362.0\XamlCompiler\Microsoft.Windows.UI.Xaml.Common.targets(482,5): error MSB4181: The "CompileXaml" task returned false but did not log an error. [C:\Users\thoma\dev\react\RN064Init2\node_modules\react-native-fs\windows\RNFS\RNFS.vcxproj]
    11>Done Building Project "C:\Users\thoma\dev\react\RN064Init2\node_modules\react-native-fs\windows\RNFS\RNFS.vcxproj" (default targets) -- FAILED.
     2>Done Building Project "C:\Users\thoma\dev\react\RN064Init2\windows\rn064init2\rn064init2.vcxproj" (default targets) -- FAILED.
    12>Done Building Project "C:\Users\thoma\dev\react\RN064Init2\windows\rn064init2\rn064init2.vcxproj.metaproj" (default targets) -- FAILED.
     1>Done Building Project "C:\Users\thoma\dev\react\RN064Init2\windows\rn064init2.sln" (default targets) -- FAILED.