Page MenuHomePhabricator

[desktop] Modify the generated native windows notifs module
ClosedPublic

Authored by michal on Mar 28 2023, 6:17 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 27, 10:25 PM
Unknown Object (File)
Wed, Mar 27, 10:25 PM
Unknown Object (File)
Wed, Mar 27, 10:25 PM
Unknown Object (File)
Wed, Mar 27, 10:25 PM
Unknown Object (File)
Wed, Mar 27, 10:25 PM
Unknown Object (File)
Wed, Mar 27, 10:24 PM
Unknown Object (File)
Wed, Mar 27, 10:11 PM
Unknown Object (File)
Fri, Mar 8, 7:00 PM
Subscribers

Details

Summary

Changes to the generated versions:

  • When running yarn install the new module will install the required dependencies from nuget
    • packages.config defines the nuget dependencies
    • This module is marked as optional so on macOS, nuget install script fails and the module is correctly ignored
  • bindings.gyp configation file has been changed because the the automatically generated one didn't work in our unpackaged app
  • There were a few changes needed in the generated c++ code itself

Also the module was added as an optional dependency of desktop. It's was not added to the workspace in root package.json because it requires nuget to build correctly and you can't have "optional" workspaces, they must all build correctly.

Test Plan

On both Windows and macOS (to check if it still builds correctly):

  • yarn cleaninstall
  • in desktop: yarn package
  • try running the resulting applications and check if they work

Additionally, for the Windows app, check using code that will be introduced in the later diff if the WNS notification channel is correctly created.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Mar 28 2023, 6:22 AM
Harbormaster failed remote builds in B17740: Diff 24280!
desktop/addons/windows-pushnotifications/_nodert_generated.cpp
32–33 ↗(On Diff #24280)

Added the includes for the bootstrapper

426–428 ↗(On Diff #24280)

This was modified to return a string containing URI directly, instead of a URI object which didn't work in the javascript world.

1658–1664 ↗(On Diff #24280)

Run the bootstrapper so we have access to the Windows API.

desktop/addons/windows-pushnotifications/binding.gyp
36 ↗(On Diff #24280)

This file was completely rewritten. This is the file that node-gyp uses to generate the msbuild solution used in the actual building of the module.

desktop/addons/windows-pushnotifications/lib/main.js
67 ↗(On Diff #24280)

I modified this file to make it pass eslint. Also there was a "hacky" integration with the automatically generated typescript so I removed as we won't need it.

desktop/addons/windows-pushnotifications/package.json
22–24 ↗(On Diff #24280)

Added nuget install. It's preinstall and not postinstall because it needs to run before yarn runs node-gyp on this module (yarn runs it automatically during yarn install).

desktop/addons/windows-pushnotifications/packages.config
1 ↗(On Diff #24280)

This file specifies nuget dependencies.

Harbormaster returned this revision to the author for changes because remote builds failed.Mar 28 2023, 6:31 AM
Harbormaster failed remote builds in B17743: Diff 24283!
michal edited the test plan for this revision. (Show Details)
  1. Can you separate the codegenned parts and your modifications into separate diffs?
  2. For the codegenned diffs, can you list the exact commands you ran to generate the code?
michal retitled this revision from [desktop] Native module for windows notifications to [desktop] Modify the generated native windows notifs module.
ashoat added inline comments.
desktop/addons/windows-pushnotifications/_nodert_generated.cpp
30–33 ↗(On Diff #24380)

Annotate and explain

425 ↗(On Diff #24380)

Annotate and explain

1656–1662 ↗(On Diff #24380)

Annotate and explain. This comment is not very useful:

Bootstrap runtime initialization

desktop/addons/windows-pushnotifications/binding.gyp
8–33 ↗(On Diff #24380)

Could you annotate each change here carefully and thoroughly so I know what's going on?

29–30 ↗(On Diff #24380)

Shouldn't we use %ProgramFiles(x86)% and %ProgramFiles% here?

desktop/package.json
26 ↗(On Diff #24380)

Can we name this @commapp/windowspush?

This revision is now accepted and ready to land.Mar 30 2023, 8:18 AM

Made bindings.gyp more general, renamed to @comapp/windowspush.

desktop/addons/windows-pushnotifications/_nodert_generated.cpp
30–33 ↗(On Diff #24697)

We need to add support for Windows bootstraper (this is a separate thing from the push notifications). We need it because we have an unpackaged app and we need to let Windows know that we will be using some native APIs. These are the needed includes. They point to the nuget package containing the windows app sdk.

425 ↗(On Diff #24697)

This was modified to return a string containing URI directly, instead of a URI object which didn't work in the javascript world. We would have done the conversion to the string immediately in js anyway so we aren't losing anything. I suspect this didn't work out of the box because the NodeRT's generated code and wrappers depend on some UWP (packaged apps) objects and apis.

1656–1661 ↗(On Diff #24697)

We need to add support for Windows bootstraper (this is a separate thing from the push notifications). We need it because we have an unpackaged app and we need to let Windows know that we will be using some native APIs. In the packaged apps this would be specified in the app manifest and Windows would do it automatically (there's a way to specify this in the project settings so msbuild generates this boilerplate automatically but I couldn't get it to work with our solution, using node-gyp and electron). We bootstrap the runtime depending on the version of the sdk, using constants from Microsoft::WindowsAppSDK.

desktop/addons/windows-pushnotifications/binding.gyp
8–17 ↗(On Diff #24697)

The bootstrap library is contained inside the Microsoft.WindowsAppSDK nuget package. Normally (when using Visual Studio) these files would be included using the .props and .target files specified in the .vcxproj file, but node-gyp doesn't support that so we have to do things manually. We add the .lib file to the linker's paths and copy the required .dll to the release directory so our module can find it.

29–31 ↗(On Diff #24697)

These are needed for the various .winmd files needed. These files specify the machine-readable metadata that describes the Windows runtime APIs. The first two lines specify the paths for windows.winMD and platform.winMD, they were updated to support building on newer versions of windows and visual studio (nodert is a bit old). The Microsoft.Windows.PushNotifications.WinMD that is being used in nodert_generated.cpp is included in the Microsoft.WindowsAppSDK nuget package so that why we add the path for it here (this whole module was generated from this winmd file, but we need to add this include because NodeRT assumes that these files are available globally for UWP apps and not just in a nuget package).

Wow this is complicated. I imagine it will be hard for somebody else to recreate this if ever necessary, but appreciate your annotations – hopefully they will be helpful

  • instead of implicitly failing the preinstall script on macOS/linux because there's no nuget installed, added a bash script that explicitly skips the package install on non-Windows platforms
  • changed one of the paths in the bindings.gyp. For some reason the old one was working when I was building the notif package by itself but stopped working when I was building the whole monorepo. The new one is already used in some PRs having a similar problem of updating to a new Visual Studio version so it's probably better anyway (although I'm not sure what the difference is...)
desktop/package.json
26 ↗(On Diff #24775)

Now that we are only running Nuget on Windows, do we still need file: here?

desktop/package.json
26 ↗(On Diff #24775)

The package installation still "fails" (exit 1 in preinstall.sh) and is then skipped because it's an optionalDependency. To use "0.0.1" this package would need to be a workspace (added in the root package.json). But we can't have "optional workspace". If one of the workspaces fails the whole yarn install will fail

desktop/package.json
26 ↗(On Diff #24775)

Should we exit 0 then?

desktop/package.json
26 ↗(On Diff #24775)

The plan was for it to fail so on non-Windows builds it's not included as a dependency (because it's "optional") of the desktop app. I've tried to make a dummy node-gyp configuration for macOS builds so that it can be built on both platforms but didn't have much luck.
That would probably be a better solution but I'm not sure if we prefer to try and figure it out right now or maybe as a follow-up.

desktop/package.json
26 ↗(On Diff #24775)

Thanks for explaining again!