Skip to content

Conversation

@jonthysell
Copy link
Contributor

@jonthysell jonthysell commented May 28, 2020

  • Updated npx react-native config to create the needed json for auto-linking
  • New npx react-native autolink-windows command to perform auto-linking
  • Auto-linking occurs automatically during npx react-native run-windows
  • New Auto-linking targets and props to catch so RNW app builds (in VS or with run-windows --no-autolink) throw a warning that auto-linking should be run
  • Update external RNW lib targets and props so run-windows works
  • Fixed run-windows to properly consume react-native config
  • Updated in-repo apps for autolinking:
    • E2ETest, SampleAppCpp, and SampleAppCS will register autolinked modules
    • playground has the generated files in the project, but does not use them
    • playground-win32 has the generated files but does not include them in the project

Closes #2853

Microsoft Reviewers: Open in CodeFlow

* Updated `npx react-native config` to create the needed json for auto-linking
* New `npx react-native autolink-windows` command to perform auto-linking
* Auto-linking occurs automatically during `npx react-native run-windows`
* New Auto-linking targets and props so CLI apps throw a warning if auto-linking should be run
* New targets and props for native modules to consume

Closes microsoft#2853
@jonthysell jonthysell requested a review from a team as a code owner May 28, 2020 20:44
@jonthysell jonthysell requested a review from asklar May 28, 2020 20:44
@NickGerleman
Copy link
Contributor

I mentioned this somewhere else, but we should really consider moving local CLI code to typescript with implicit any sooner than later. Types can be incrementally added, and we get way better readability, dev experience, the ability to use ES6 import and export, etc.

@NickGerleman
Copy link
Contributor

There has been a lot of recent conversation about how we need better test coverage. Any ideas on how we might be able to build validation for this change?

@jonthysell
Copy link
Contributor Author

There has been a lot of recent conversation about how we need better test coverage. Any ideas on how we might be able to build validation for this change?

The current CI will already capture the case of a CLI app where there are no community modules installed. Autolinking will run, and everything should still work.

At some point we'll want a CI task that creates a new app, adds a community module, builds and verifies that autolinking worked. However to do that we need to get this in, and backported into a release, and get a community module that is willing/able to upgrade to the RN version we've backported to, then make sure the windows implementation is upgraded and working too.

In that respect I'm working on a PR for react-native-camera, getting it to build against 0.62, but they are built on RN 0.59, and I'm not sure yet how to make the windows implementation build on both 0.62 and anything earlier, since 0.62 changed a lot build-wise.

Copy link
Member

@asklar asklar left a comment

Choose a reason for hiding this comment

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

Mainly I want to make sure we have the right docs to make it clear for folks how this works. My feedback, prioritized:
P1: Make sure we have a good doc on this explaining behavior and fallbacks.
P2: Don't rely on side effects of using the app targets Import in order to infer which one is the app project, just use an actual property on the project and use that.
P2: Ideally we don't use our own regex based matching for parsing XML and we instead use something like xml2js or node-xml
P2/P3: other minor feedback

@ghost ghost added Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) and removed Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) labels Jun 11, 2020
@jonthysell
Copy link
Contributor Author

I can't check-in the docs until the 0.62 releases and the website version is bumped.

Here's the PR: microsoft/react-native-windows-samples#145

Here are the relevant docs:

@jonthysell jonthysell requested a review from asklar June 15, 2020 18:34
<AutolinkCommand Condition="'$(AutolinkCommand)' == ''">npx react-native autolink-windows</AutolinkCommand>
<AutolinkCommandArgs Condition="'$(AutolinkCommandArgs)' == ''">--check</AutolinkCommandArgs>
<AutolinkCommandWorkingDir Condition="'$(AutolinkCommandWorkingDir)' == ''">$([MSBuild]::GetDirectoryNameOfFileAbove($(ProjectDir), 'package.json'))</AutolinkCommandWorkingDir>
<AutolinkCommandArgs Condition="'$(AutolinkCommandArgs)' == '' And '$(SolutionPath)' != '' And '$(ProjectPath)' != ''">--check --sln $([MSBuild]::MakeRelative($(AutolinkCommandWorkingDir), $(SolutionPath))) --proj $([MSBuild]::MakeRelative($(AutolinkCommandWorkingDir), $(ProjectPath)))</AutolinkCommandArgs>
Copy link
Member

Choose a reason for hiding this comment

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

who's setting SolutionPath and ProjectPath?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this as an extra safeguard to make sure the check runs accurately for playground (and potential other multi-solution or multi-app project setups). It's conditional on SolutionPath and ProjectPath existing. Otherwise, it falls through to the default args which is to just use the result from react-native config.

Alternatively I could just override the args prop in playground projects, but I didn't want to make such a change in the template for all apps.

I know running msbuild directly against a project doesn't set the solution path. But using VS or run-windows, it gets set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not 100% happy with it, as we should be relying entirely on react-native config, but this was the best compromise I could come up with for multi solution setups.

Copy link
Member

Choose a reason for hiding this comment

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

@jonthysell thanks for the explanation. I can't find how it gets set through run-windows though (I assume VS has its own way of invoking msbuild which sets these variables up), but since we're invoking msbuild manually I would have expected us to pass /p:... or something like that

@jonthysell jonthysell merged commit 78ae737 into microsoft:master Jun 19, 2020
@jonthysell jonthysell deleted the autolink branch June 22, 2020 16:02
jonthysell added a commit to microsoft/react-native-windows-samples that referenced this pull request Jun 25, 2020
* Added docs for autolinking added in RNW 0.63. See microsoft/react-native-windows#5044
* Fixed unbroken errors
matsudaWWW added a commit to matsudaWWW/reactNative-sample that referenced this pull request Dec 17, 2023
* Added docs for autolinking added in RNW 0.63. See microsoft/react-native-windows#5044
* Fixed unbroken errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: Infrastructure 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.

Implement auto linking

5 participants