Skip to content

Conversation

@JunielKatarn
Copy link
Contributor

@JunielKatarn JunielKatarn commented Apr 8, 2023

Description

Replacing WinRT with Boost for GUID generation.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

Why

winrt::Windows::Foundation::GuidHelper::CreateNewGuid() is not available on Windows versions 10.0.17134 and earlier, breaking the Blob module storage on those releases.

What

Drop usage of C++/WinRT GUID generation APIs with Boost APIs.

Testing

  • Desktop Integration Tests (Blob)
Microsoft Reviewers: Open in CodeFlow

@JunielKatarn JunielKatarn added the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label Apr 8, 2023
@JunielKatarn JunielKatarn requested a review from a team April 9, 2023 00:41
@asklar
Copy link
Member

asklar commented Apr 9, 2023

we don't support 10.0.17134 and earlier for RNW, and bringing in boost headers usually makes the build longer. can this be gated for office only?

@JunielKatarn JunielKatarn removed the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label Apr 10, 2023
@JunielKatarn
Copy link
Contributor Author

we don't support 10.0.17134 and earlier for RNW, and bringing in boost headers usually makes the build longer. can this be gated for office only?

I'll measure the build times over several iterations.
If impact is null or negligible, I'd rather not maintain separate implementations.

Moreover, the WinRT APIs require string type conversion (wchar to char) so the Boost API may even be more efficient at runtime for our use case.

@JunielKatarn
Copy link
Contributor Author

JunielKatarn commented Apr 11, 2023

I tried 10 builds with and without my changes based on the latest main as of today, with the following settings:

  • Same workstation
  • Building Microsoft.ReactNative.dll
  • Pre-filled NuGet cache and Yarn setup commands
  • Removed vnext\build and vnext\target directories before each iteration
  • Build favor x64|Release

The build time difference is virtually non-existing.
This particular use of Boost is not impacting build performance empirically.
Merging as is to avoid diverging implementations without a noticeable build performance benefit.
image

@JunielKatarn JunielKatarn merged commit 0a53092 into microsoft:main Apr 11, 2023
@JunielKatarn JunielKatarn deleted the OC/7771855/mkguid branch April 11, 2023 01:54
JunielKatarn added a commit to jurocha-ms/react-native-windows that referenced this pull request Apr 11, 2023
* Update package lock

* Use Boost UUID library

* Change files

* Remove GuidHelper import
JunielKatarn added a commit to jurocha-ms/react-native-windows that referenced this pull request Apr 11, 2023
* Update package lock

* Use Boost UUID library

* Change files

* Remove GuidHelper import
JunielKatarn added a commit to jurocha-ms/react-native-windows that referenced this pull request Apr 11, 2023
* Update package lock

* Use Boost UUID library

* Change files

* Remove GuidHelper import
JunielKatarn added a commit to jurocha-ms/react-native-windows that referenced this pull request Apr 11, 2023
* Update package lock

* Use Boost UUID library

* Change files

* Remove GuidHelper import
JunielKatarn added a commit that referenced this pull request Apr 11, 2023
* Use Boost GUID generator (#11470)

* Update package lock

* Use Boost UUID library

* Change files

* Remove GuidHelper import

* Change files
JunielKatarn added a commit that referenced this pull request Apr 11, 2023
* Use Boost GUID generator (#11470)

* Update package lock

* Use Boost UUID library

* Change files

* Remove GuidHelper import

* Change files
JunielKatarn added a commit that referenced this pull request Apr 11, 2023
* Use Boost GUID generator (#11470)

* Update package lock

* Use Boost UUID library

* Change files

* Remove GuidHelper import

* Change files
JunielKatarn added a commit that referenced this pull request Apr 11, 2023
* Use Boost GUID generator (#11470)

* Update package lock

* Use Boost UUID library

* Change files

* Remove GuidHelper import

* Change files
JunielKatarn added a commit to jurocha-ms/react-native-windows that referenced this pull request Jul 14, 2023
* Update package lock

* Use Boost UUID library

* Change files

* Remove GuidHelper import
JunielKatarn added a commit that referenced this pull request Jul 18, 2023
* Use Boost GUID generator (#11470)

* Update package lock

* Use Boost UUID library

* Change files

* Remove GuidHelper import

* Bump C++ Windows Version macros to Windows 10 (#11516)

* Bump default _WIN32_WINNT_VERSION

* Change files

* Remove unused Beast-dependent code (#11517)

* Remove unused Beast production code

* Change files

* clang format

---------

Co-authored-by: Andrew Coates <30809111+acoates-ms@users.noreply.github.com>

* Define FileReader turbo module (#11789)

* Declare FileReaderTurboModule

* Declare m_blobPersistor

* Use msrn namespace alias in FileReader module

* json lock

* Update lock files

* config

* Implement ReadAsDataUrl

* Define ReadAsText

* Define FileReaderResource

* Move persistor from module into resource

* Rename module name name

* Add using ModuleSpec

* clang format

* Change files

* Define WebSocket TurboModule (#11802)

* Add WSTurboModule.h

* Implement CreateResource and MSRN SendEvent

* Define IWebSocketModuleContentHandler::ProcessMessage with JSValueObject

* Complete WebSocketTurboModule::CreateResource

* Complete WebSocketTurboModule::Connect

* Implement close, send, sendBinary and ping

* Include NativeWebSocketModuleSpec

* Make Close spec-compliant

* Add spec-compliant method stubs

* Make remaining existing methods spec-compliant

* Move NOOP Add/RemoveListeners implementation into source file

* clang format

* Use double as keys for m_resourceMap

* Change files

* Define HTTP TurboModule (#11825)

* Declare TurboModule

* Generate NativeNetworkingWindows spec

* Define HttpTurboModule

* Begin resource setup

* Initialize: Up to SetOnData

* Define wchar_t event names

* Migrate IRequestBodyHandler to JSValue

IMPORTANT: Migrate and fix RequestArgs::Data

* Migrate IResponseHandler to JSValue

* Rename m_onDataDynamic as m_onDataObject

* Implement Microsoft::React::Modules::ToJSValue

* Migrate IHttpResource to JSValue

* Fix Playground flavors ordering

* Finish implementing Initialize

* Define ToDynamic

* Change files

* Revert module.g.cpp

* Revert "Revert module.g.cpp"

This reverts commit 2f6c88a.

* Define target SetCppWinRTReferences

* clang format

* Revert temporary symbol exports

* Fix RedirectHttpFilterUnitTest import order

* Preserve include ordering in RedirectHttpFilterUnitTest.cpp

* Fix JSValueArray list constructor

* Drop RCTNetworkingNative in favor of NativeNetworkingWindows

* Add NativeNetworkingWindows to overrides

* Fix overrides.json newline

* Desktop int. tests: set RN_PLATFORM to win32

* Use iterator to lookup content types

* Revert module.g.cpp to main

* Use iterator for ToRequestBody content type

* Drop namespace from JSValueObject usage

* Finish HttpTurboModule

* clang format

* Use iOS HTTP spec

* Use DynamicReader and DynamicWriter for folly conversions

* Revert overrides.json

* Don't add newline to overrides.json

* Make RCTNetworking.windows.js a copy of RCTNetworking.ios.js

* Override upgrade

* Default IncludeFabricInterface to true (#11835)

* Default IncludeFabricInterface to true

* Change files

* Remove IncludeFabricInterface from ExperimentalFeatures

* Define Blob TurboModule (#11842)

* REVERT: remove CompositionView exports

* Declare BlobTurboModule

* Revert "REVERT: remove CompositionView exports"

This reverts commit 1ba874e.

* Define BlobTurboModule

* Declare IBlobResource

* Define DefaultBlobResource

* Define BlobCallbacks

* Migrate BlobWebSocketModuleContentHandler

* Pass needed args to DefaultBlobResource

* Define IBlobResource +/- network handlers

* Implement BlobModule using IBlobResource

* Fix GetConstants ownership

* Change files

* Set up error callback!

* clang format

* Remove BlobModule::SharedState

* Clean up includes and usings

* Remove ProcessMessage folly variant

* Remove folly:dynamic references

* Define IBlobResource::FieldNames

* Move IBlobPersistor out of Modules/

* Remove TODO comments

* clang format

* Add missing include to DefaultBlobResource

* Change multi_threaded_map to single_threaded_map in WinRTHttpResource (#11604)

* Change multi_threaded_map to single_threaded_map in WinRTHttpResource

* Change files

* Fix fetch failing with certain custom user agents (#11775)

* Fix fetch failing with certain custom user agents

* Change files

* Use TurboModules for networking in MSRN (#11867)

* Update packages.json

* Define GetHttpTurboModuleName

* Update packages.json

* Define GetHttpModuleProvider

* Define other netwk modules provider methods

* Register networking turbomodules

* Change files

* clang format

* Uncomment MSRN forward declarations

* Remove cntPtr.h include

* Remove CoreNativeModules

* Revert packages.lock.json

* Remove change files

* Change files

* Revert lock files

* Use correct data types in FileReaderTurboModule::ReadAs* (#11902)

* Use ReadAs* data argument as JSValueObject

* Change files

* Remove change files

---------

Co-authored-by: Andrew Coates <30809111+acoates-ms@users.noreply.github.com>
Co-authored-by: Liron Yahdav <lyahdav@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants