New Implementation of RNFS for Windows #952

avmoroz · 2020-11-14T05:46:09Z

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.

jeanmaried

Testing out the exists and readFile methods I found that they fail and crash the app when the file path points to a non-existent file. In my case I have a file located at DocumentDirectoryPath + properties.json. When the error is thrown, the catch that is supposed to return a promise.Resolve(false) is never called.

jeanmaried · 2021-05-19T20:11:55Z

windows/RNFS/RNFSManager.cpp

+        promise.Resolve(true);
+    }
+}
+catch (const hresult_error& ex)


This catch is never called if the filepath points to a non-existent path. These are the type of exceptions in the output:

Exception thrown at 0x00007FFED89D4B89 (KernelBase.dll) in MyApp.exe: WinRT originate error - 0x80070002 : 'The system cannot find the file specified.'. Exception thrown at 0x00007FFED89D4B89 in MyApp.exe: Microsoft C++ exception: winrt::hresult_error at memory location 0x0000003BE39FD5F0.

jeanmaried · 2021-05-19T20:13:25Z

windows/RNFS/RNFSManager.cpp

+    winrt::hstring base64Content{ Cryptography::CryptographicBuffer::EncodeToBase64String(buffer) };
+    promise.Resolve(winrt::to_string(base64Content));
+}
+catch (const hresult_error& ex)


This catch is also not reached if the file path points to non-existent file. These are the type of exceptions in the output:

Exception thrown at 0x00007FFED89D4B89 (KernelBase.dll) in MyApp.exe: WinRT originate error - 0x80070002 : 'The system cannot find the file specified.'. Exception thrown at 0x00007FFED89D4B89 in MyApp.exe: Microsoft C++ exception: winrt::hresult_error at memory location 0x0000003BE39FD5F0.

I noticed yesterday also these The system cannot find the file specified errors in my tests.

I tested now more exists method and it seems that error goes to catch block. There is still error in console Exception thrown at 0x00007FFED89D4B89 (KernelBase.dll) in MyApp.exe: WinRT originate error - 0x80070002 : 'The system cannot find the file specified.' maybe it is ok?

exists catch block has bug in error hanlding.

tero-paananen · 2021-05-20T05:58:44Z

Is there bug in error handling for example in RNFSManager::exists ?

catch (const hresult_error& ex)
{
    hresult result{ ex.code() };
    if (result == HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND)) {
        promise.Resolve(false);
    }
    // "Failed to check if file or directory exists.
    promise.Reject(winrt::to_string(ex.message()).c_str());
}

code calls promise twice, first promise.Resolve(false); and then promise.Reject(winrt::to_string(ex.message()).c_str()); if error is that ERROR_FILE_NOT_FOUND

tero-paananen · 2021-05-20T06:38:59Z

When RNFS receives lot of Exception thrown at 0x00007FFED89D4B89 (KernelBase.dll) in MyApp.exe: WinRT originate error - 0x80070002 : 'The system cannot find the file specified. errors is my Visual Studio debug always stopped. I had to press Continue to let app run again. This is not very developer friendly? I had to remove this winrt:hresult_error error from list of Break When Thrown to not always stop this. I do not know what is right solution for this.

jeanmaried · 2021-05-20T17:55:20Z

When RNFS receives lot of Exception thrown at 0x00007FFED89D4B89 (KernelBase.dll) in MyApp.exe: WinRT originate error - 0x80070002 : 'The system cannot find the file specified. errors is my Visual Studio debug always stopped. I had to press Continue to let app run again. This is not very developer friendly? I had to remove this winrt:hresult_error error from list of Break When Thrown to not always stop this. I do not know what is right solution for this.

This is the issue that I'm having.

exotexot · 2021-05-26T14:43:13Z

This is also a observation I have made. If this could be fixed appropriately, that would be awesome! @avmoroz

exotexot · 2021-07-27T09:09:54Z

Any updated happening here? :(

asklar · 2021-07-27T10:00:14Z

@chiaramooney could you take on driving to get this merged?

raaannzzz · 2021-07-29T19:49:06Z

+1

emanuelmartin · 2021-07-29T21:02:07Z

@chiaramooney could you take on driving to get this merged?

So happy to hear that 💯

chiaramooney · 2021-08-09T17:13:28Z

@itinance any concerns merging this?

namrog84 · 2021-10-01T00:45:27Z

Any potential update on this getting in? Would love to see it happen!

emanuelmartin · 2021-10-01T19:20:46Z

We need a GIF saying "Merge it mate"

exotexot · 2021-11-28T23:35:17Z

Anyone tested this with a recent react-native version under Windows 11 ?

datacana · 2021-11-30T22:24:10Z

sadly no. example cannot build. does throw errors in react native JsiApiContext.cpp

@asklar @avmoroz any ideas?

asklar · 2021-11-30T22:30:29Z

sadly no. example cannot build. does throw errors in react native JsiApiContext.cpp

@asklar Alexander Sklar FTE @avmoroz any ideas?

See microsoft/react-native-windows#7585 (comment)

namrog84 · 2021-12-03T02:15:58Z

windows/RNFS/RNFS.def

@@ -0,0 +1,3 @@
+EXPORTS
+DllCanUnloadNow = WINRT_CanUnloadNow                    PRIVATE


I was testing this PR in our code via a patch and was getting weird error around this file when it wasn't using crlf (it appears to be currently lf) Visual Studio 2019 compiler errored out on it.

Consider updating to crlf?

namrog84 · 2022-01-03T21:15:57Z

@asklar, @avmoroz, @tero-paananen

Is there anything still blocking this branch from going in?

It'd be awesome to just get it in, and then would be easier to fix bugs/make PRs to it then.

asklar · 2022-01-04T03:29:22Z

@asklar, @avmoroz, @tero-paananen

Is there anything still blocking this branch from going in?

It'd be awesome to just get it in, and then would be easier to fix bugs/make PRs to it then.

No concerns from me to merge and fix bugs separately as the pr is already large. However I don't know how much appetite @itinance might have to merge this (as it's been open for a long time), and if further changes are required to fix bugs, I'd be concerned about the velocity with which changes are accepted. So perhaps a fork might be required.

creambyemute · 2022-01-06T20:45:28Z

I'm currently facing a problem with user-directory paths that contain Umlaute/accentuated characters

Maybe someone has an idea how to handle the std::string --> std::path conversion correctly?

microsoft/react-native-windows#9315

creambyemute · 2022-01-07T17:08:07Z

@asklar @namrog84 @avmoroz FYI
I was testing my std::string --> path withUmlaute problem today and got the app working with a few rather small changes in RNFS:

I created a convertPath method and adapted the widely used splitPath method as well.
With a few more changes (using convertPath(...) where splitPath(...) isn't used) + a change to downloadFile(...) (path.string() instead of path.c_str()) it started working.

I don't like this solution yet (as it probably isn't correct?) and it can most likely be further narrowed down and simplified.

std::filesystem::path RNFSManager::convertPath(const std::filesystem::path originalPath) 
{
    winrt::hstring fPathHString = winrt::to_hstring(originalPath.string());
    std::filesystem::path path{fPathHString.c_str()};
    path.make_preferred();
    return path;
}


void RNFSManager::splitPath(const std::string& fullPath, winrt::hstring& directoryPath, winrt::hstring& fileName) noexcept
{
    std::filesystem::path path(fullPath);
    path.make_preferred();
    std::filesystem::path convertedPath = convertPath(path);

    directoryPath = convertedPath.has_parent_path() ? winrt::to_hstring(convertedPath.parent_path().c_str()) : L"";
    fileName = convertedPath.has_filename() ? winrt::to_hstring(convertedPath.filename().c_str()) : L"";
}

winrt::fire_and_forget RNFSManager::downloadFile(...) noexcept
{
    auto filePath{ winrt::to_hstring(path.string()) };
}

// In mkdir and some others
std::filesystem::path firstPath(pathString);
firstPath.make_preferred();
std::filesystem::path path = convertPath(firstPath);

Edit:
After some more tests, it seems that the following is enough when directly using the filesystem::path with UWP/winRT apis.

std::filesystem::path fPath{ stringPath };
fPath.make_preferred();
StorageFolder::GetFolderFromPathAsync(fPath.string())

// Also works
StorageFolder::GetFolderFromPathAsync(fPath.string().c_str())

Edit2:
After some more tests, it seems even better to pass std::wstring instead of std::string as path from JS --> RN-Windows

std::filesystem::path fPath{ wstringPath };
fPath.make_preferred();
StorageFolder::GetFolderFromPathAsync(fPath.wstring())

creambyemute · 2022-01-17T14:01:20Z

@namrog84 FYI: I have created a branch (fixUmlautePaths) and PR on https://github.com/wwimmo/react-native-fs if your team wants to try out the changes

adamgorMSFT · 2022-03-31T20:30:51Z

Is there anything that needs to happen to help this PR get in? I am sure there are some bugs that need fixing that I'd help contribute to fixes, but I know this PR is massive and going to cause a little bit of hiccup for a moment.

taduyde · 2022-07-15T11:57:02Z

when we merge this branch?

shanelile · 2022-10-20T19:56:44Z

Are there any updates to this? Is it blocked by anything?

avmoroz added 30 commits July 7, 2020 20:48

Implement unit tests and methods

1be2241

Update .gitignore

abb1930

Implement Unit Tests

96e3690

Implements additional unit tests

60302d1

Implement additional unit tests

1e8c3a8

Implement stat

82fd113

Implement hash method

f2aa4f4

Add Initial Download

e8b0542

Add Demo App functionality

9f2d471

Repair expected functionality

6055684

Fix Stat functionality

ed30f0c

Added fixes and functionality for Demo App

e4fe048

Implement syntax fixes and tests

0e50237

Implemented downloadFile

8aea2e2

Started (not finished) UploadFiles

f1ce0fc

Updated downloadFile in demo app

716e141

Implement bare-bones working uploadFiles

06f05fb

Added disposition information

cf55697

Implement file transfer via multipart HTTP request

3c79655

implement parsing of fields and select headers

33991fb

Rename variables for clarity

52071ec

Implement progress tracking for uploadFiles

fa432b4

Updated tests and variable names

af50f60

Move windows C# code to windows_legacy

00a8057

Added progressInterval value

8301d83

Implement hotfix for progressIncrement in downloadFile

9ea16ca

Modify copyFile and moveFile to work as intended

1a39cd3

Modified error messages

3b088dd

Remove temp files

143d332

Updated rejects with ReactErrors

582b0af

jeanmaried reviewed May 19, 2021

View reviewed changes

chrisglein mentioned this pull request Jul 15, 2021

auto link not happening for the package in react native windows microsoft/react-native-windows#8227

Closed

asklar mentioned this pull request Aug 21, 2021

How can i add react-native-fs module to the react-native-windows project ? microsoft/react-native-windows#8442

Closed

tylerforsythe mentioned this pull request Sep 28, 2021

Unable to get property RNFSFileTypeRegular of undefined or null reference. #1039

Open

asklar mentioned this pull request Oct 15, 2021

[0.66] react-native-fs does not properly link even though it seems to have windows microsoft/react-native-windows#8836

Closed

namrog84 reviewed Dec 3, 2021

View reviewed changes

creambyemute mentioned this pull request Jan 7, 2022

[Windows] Possible problems with paths with Umlaute/accentuated characters RonRadtke/react-native-blob-util#97

Closed

itinance merged commit e8e092c into itinance:master Apr 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Implementation of RNFS for Windows #952

New Implementation of RNFS for Windows #952

avmoroz commented Nov 14, 2020

jeanmaried left a comment •

edited

jeanmaried May 19, 2021

jeanmaried May 19, 2021

tero-paananen May 20, 2021

tero-paananen May 20, 2021 •

edited

tero-paananen commented May 20, 2021 •

edited

tero-paananen commented May 20, 2021 •

edited

jeanmaried commented May 20, 2021

exotexot commented May 26, 2021

exotexot commented Jul 27, 2021

asklar commented Jul 27, 2021

raaannzzz commented Jul 29, 2021

emanuelmartin commented Jul 29, 2021

chiaramooney commented Aug 9, 2021

namrog84 commented Oct 1, 2021

emanuelmartin commented Oct 1, 2021

exotexot commented Nov 28, 2021

datacana commented Nov 30, 2021

asklar commented Nov 30, 2021

namrog84 Dec 3, 2021

namrog84 commented Jan 3, 2022 •

edited

asklar commented Jan 4, 2022

creambyemute commented Jan 6, 2022

creambyemute commented Jan 7, 2022 •

edited

creambyemute commented Jan 17, 2022

adamgorMSFT commented Mar 31, 2022

taduyde commented Jul 15, 2022

shanelile commented Oct 20, 2022

		@@ -0,0 +1,3 @@
		EXPORTS
		DllCanUnloadNow = WINRT_CanUnloadNow PRIVATE

New Implementation of RNFS for Windows #952

New Implementation of RNFS for Windows #952

Conversation

avmoroz commented Nov 14, 2020

jeanmaried left a comment • edited

Choose a reason for hiding this comment

jeanmaried May 19, 2021

Choose a reason for hiding this comment

jeanmaried May 19, 2021

Choose a reason for hiding this comment

tero-paananen May 20, 2021

Choose a reason for hiding this comment

tero-paananen May 20, 2021 • edited

Choose a reason for hiding this comment

tero-paananen commented May 20, 2021 • edited

tero-paananen commented May 20, 2021 • edited

jeanmaried commented May 20, 2021

exotexot commented May 26, 2021

exotexot commented Jul 27, 2021

asklar commented Jul 27, 2021

raaannzzz commented Jul 29, 2021

emanuelmartin commented Jul 29, 2021

chiaramooney commented Aug 9, 2021

namrog84 commented Oct 1, 2021

emanuelmartin commented Oct 1, 2021

exotexot commented Nov 28, 2021

datacana commented Nov 30, 2021

asklar commented Nov 30, 2021

namrog84 Dec 3, 2021

Choose a reason for hiding this comment

namrog84 commented Jan 3, 2022 • edited

asklar commented Jan 4, 2022

creambyemute commented Jan 6, 2022

creambyemute commented Jan 7, 2022 • edited

creambyemute commented Jan 17, 2022

adamgorMSFT commented Mar 31, 2022

taduyde commented Jul 15, 2022

shanelile commented Oct 20, 2022

jeanmaried left a comment •

edited

tero-paananen May 20, 2021 •

edited

tero-paananen commented May 20, 2021 •

edited

tero-paananen commented May 20, 2021 •

edited

namrog84 commented Jan 3, 2022 •

edited

creambyemute commented Jan 7, 2022 •

edited