Skip to content

Conversation

@acoates-ms
Copy link
Contributor

@acoates-ms acoates-ms commented May 11, 2020

Now that we have PropertyBag we can implement more of the built in native modules using the spec files.

This moves the DevSettings implementation over to using the spec file.

In doing so, 3 new methods were added to the implementation which had been added since 61, and missed in the 62 update.

Waiting on #4870 to allow DevSettings to hold on to ReactContext, rather than using Handle and IReactContext.
Right now there is a fairly ugly work needed to store a function in the property bag. - This can probably be improved in the future. -- Unless someone knows of an existing winrt mechanism of wrapping a function pointer in IInspectable. (winrt:delegate still uses IUnknown)

Microsoft Reviewers: Open in CodeFlow

@acoates-ms acoates-ms requested a review from vmoroz May 11, 2020 20:04
@acoates-ms acoates-ms requested a review from a team as a code owner May 11, 2020 20:04
@aeulitz
Copy link
Contributor

aeulitz commented May 11, 2020

          L"Clipboard",

For my understanding, is this something which in the long run we would want to factor out?


Refers to: vnext/Microsoft.ReactNative/ReactHost/ReactInstanceWin.cpp:217 in b4f83e0. [](commit_id = b4f83e0, deletion_comment = False)

Copy link
Contributor

@aeulitz aeulitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@acoates-ms
Copy link
Contributor Author

          L"Clipboard",

For my understanding, is this something which in the long run we would want to factor out?

Refers to: vnext/Microsoft.ReactNative/ReactHost/ReactInstanceWin.cpp:217 in b4f83e0. [](commit_id = b4f83e0, deletion_comment = False)

For non-core modules the registration will be auto generated as part of auto linking. Within core its less clear that we will do that. Its not something that is likely to get out of sync, since if they rename the module the spec file would get renamed and we've have to touch the code anyway.


private:
Mso::VoidFunctor m_reload;
static winrt::Microsoft::ReactNative::IReactPropertyName ReloadProperty() noexcept;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

winrt::Microsoft::ReactNative::IReactPropertyName [](start = 9, length = 49)

Since this method is private, consider using the ReactPropertyId instead to have a strongly typed access.

Copy link
Member

@vmoroz vmoroz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@acoates-ms acoates-ms added the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label May 12, 2020
@ghost
Copy link

ghost commented May 12, 2020

Hello @acoates-ms!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 8033b54 into microsoft:master May 12, 2020
@acoates-ms acoates-ms deleted the devsettingsspec branch May 12, 2020 21:42
ghost pushed a commit that referenced this pull request May 14, 2020
* improve treedump capabilities

* Reenable V8 for desktop projects (#4840)

* Delay load ChakraCore.dll

* Change files

* clean up unneeded project references

* Overwrite property after React.Cpp.props gets included

* Change files

* change file

* Fix WinUI3

* Reverse the conditional logic

* Revert some unnecessary changes. Consolidate the preprocessor defines in the props

Co-authored-by: tudorm <[email protected]>

* applying package updates ***NO_CI***

* update masters

* Improve run_wdio to print out failed tests (#4843)

* Improve run_wdio to print out failed tests

* Fire onLoad event when a bitmap image is opened (#4750)

* Fire onLoad event when a bitmap image is opeed

* Change files

* add imageFailed

* remove firing of onload events in the createImageBrush block so that load events don't fire twice.

* Expose ability for apps to provide their own RedBox implementation (#4786)

* Expose ability for apps to provide their own RedBox implementation

* Change files

* minor changes

* Code review feedback

* minor fix

* format fixes

* minor change

* minor fix

* minor fix

* minor fix

* Bump fp-ts from 2.5.4 to 2.6.0 (#4871)

Bumps [fp-ts](https://github.com/gcanti/fp-ts) from 2.5.4 to 2.6.0.
- [Release notes](https://github.com/gcanti/fp-ts/releases)
- [Changelog](https://github.com/gcanti/fp-ts/blob/master/CHANGELOG.md)
- [Commits](gcanti/fp-ts@2.5.4...2.6.0)

Signed-off-by: dependabot-preview[bot] <[email protected]>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

* applying package updates ***NO_CI***

* Fixed ReactContext copy/move semantic (#4870)

* Fixed ReactContext copy/move semantic

* Change files

* Fixed formatting

* Test to show how to use a delegate in ReactPropertyBag

* Add ReactNativeHost to Win32 C++/WinRT ABI  (#4848)

* create MS.ReactNative.IntegrationTests project

* RNHost activation succeeds

* update

* update

* Change files

* add JS function call test

* Change files

* fix formatting

* submit current state

* ReactNativeHost activation test succeeds

* Change files

* account for property bag addition

* sync, address code review feedback

* Use spec file for DevSettings NativeModule (#4873)

* Use spec file for DevSettings NativeModule

* Change files

* minor change

* Handle HTTP errors in DevSupportManager. (#4880)

* Handle HTTP connect exceptions in DevSupportManager

* Change files

* applying package updates ***NO_CI***

* Allow storing non-WinRT types in ReactPropertyBag (#4884)

* Allow storing non-WinRT types in ReactPropertyBag

* Change files

* Addressed PR feedback

* Use DispatcherQueue instead of CoreDispatcher for Mso::DispatchQueue (#4877)

* Use DispatcherQueue instead of CoreDispatcher for Mso::DispatchQueue

* Change files

* Removed 'Thread' from some new API names.

* Removed extra 'private' section in AppearanceModule

* Temporary: use CoreDispatcher to check the E2E test

* Avoid using DispatcherQueue::HasThreadAccess in earlier Windows versions.

* RNW dependencies (#4876)

* add rnw-dependencies.ps1

* Change files

* --sln

* run in CI

* print

* desktop appium is optional

* use agent directory

* 15GB is a ballpark estimate

* optional space

* .

* support running from web (#4892)

* Bump typescript from 3.8.3 to 3.9.2 (#4890)

* Bump lerna from 3.20.2 to 3.21.0 (#4889)

* BugFix:  fix default tabindex stomping over acceptsKeyboardFocus (#4893)

* Change files

* revert dfc57fc

* set default tab index to -1, the hope is to be in line with xbox assumption

* xbox doesn't seem to rely on this, changing default back to match xaml default

* Update e2e testing doc with CI debugging info (#4897)

* add instructions for CI debugging for e2e test app

* point e2e readme to e2e testing doc

* Update rnw-dependencies  (#4894)

* don't exit the powershell session and pause for keyboard input if interactive

* Change files

* enable Switch (fixed 4596)

* improve treedump capabilities

* update masters

* enable Switch (fixed 4596)

* protect against exceptions in run_wdio

* add another try block

* update e2e testing and masters, fixes 4680

* publish wdio report and fix run_wdio typo bug

* TreeDump should ignore collapsed object elements in an array

* add info about run_wdio

Co-authored-by: tudorms <[email protected]>
Co-authored-by: tudorm <[email protected]>
Co-authored-by: React-Native-Windows Bot <[email protected]>
Co-authored-by: lamxdoan <[email protected]>
Co-authored-by: Andrew Coates <[email protected]>
Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: Vladimir Morozov <[email protected]>
Co-authored-by: Andreas Eulitz <[email protected]>
Co-authored-by: Julio César Rocha <[email protected]>
Co-authored-by: kmelmon <[email protected]>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants