Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove ReactWindowsCore projects #5012

Merged
merged 48 commits into from
May 29, 2020
Merged

Conversation

JunielKatarn
Copy link
Contributor

@JunielKatarn JunielKatarn commented May 27, 2020

Source folder and projects ReactWindowsCore have become redundant, and only increase project structure complexity:

  • A shared (VCXITEMS) project already exists and is used for its current purpose: Shared\Shared.vcxitems.
  • Since Desktop and Universal builds take place in different solutions, there is currently no shared static library that would reduce build times.
  • Since Desktop and Universal have different settings for Hermes and V8, two separate static libs had been created: ReactWindowsCore.vcxproj and ReactWindowsCore-Desktop.vcxproj.
    These were directly consumed ONLY by their corresponding Desktop and Universal projects, thus adding no build benefit over a shared items project (i.e. Shared.vcxitems).

This change simplifies the solution structure moving the shared source files into their intended project.

Note: This change seems rather large, but it's only because sources are moved from the ReactWindowsCore subdirectory to Shared.

Microsoft Reviewers: Open in CodeFlow

@JunielKatarn JunielKatarn requested review from vmoroz, asklar, acoates-ms and a team May 27, 2020 02:15
@JunielKatarn JunielKatarn requested a review from a team as a code owner May 27, 2020 02:15
@JunielKatarn JunielKatarn requested a review from asklar May 27, 2020 23:52
@JunielKatarn
Copy link
Contributor Author

@vmoroz I have scaled down the size of this change:

  • Removed ONLY the ReactWindowsCore project, but left the sources in place so the change is not deemed as "too big". Will revisit moving the sources after we start working on 0.63.
  • Reverted the Desktop root namespace renaming. Will submit it on a separate change.

C.C. @asklar

@@ -4,7 +4,7 @@
#include <winrt/facebook.react.h>

namespace facebook::react {
class MessageQueueShim : public MessageQueueThread {
class MessageQueueShim : public facebook::react::MessageQueueThread {
Copy link
Member

Choose a reason for hiding this comment

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

revert?

Copy link

@NikoAri NikoAri 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 merged commit ff72d1b into microsoft:master May 29, 2020
@JunielKatarn JunielKatarn deleted the fixcommon branch May 29, 2020 03:33
@jonthysell
Copy link
Contributor

@JunielKatarn Please make a corresponding update to the docs repo for native modules setup to indicate that this project is no longer present nor necessary. If this is getting backported to 62, you'll want to make these changes now, before the website releases the 62 docs. Otherwise, you want to wait until after the website snaps to 62, and then update the docs.

@JunielKatarn
Copy link
Contributor Author

@jonthysell certainly. I'll address this today.

@NickGerleman
Copy link
Collaborator

Removing the Backport to 0.62 label, since we need to rename it as part of the deprecation in favor of Request Backport which was already added to this PR. Triage of this item should still happen soon.

@Khalef1
Copy link
Contributor

Khalef1 commented Jun 8, 2020

We discussed this in triage and decided not to backport the change. It's nice to have but will also cause a breaking change to the template project

@jonthysell jonthysell added the Breaking Change This PR will break existing apps and should be part of the known breaking changes for the release label Jun 9, 2020
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) Breaking Change This PR will break existing apps and should be part of the known breaking changes for the release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants