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

Rewrite using new Aperture and n-api #34

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

karaggeorge
Copy link
Member

@karaggeorge karaggeorge commented Nov 11, 2024

Implements the changes in wulkano/Aperture#80

  • Adds support for the new APIs
  • Allows multiple recorders to be running at the same time
  • Adapts the TS types to support multiple recording types
  • Adds a Node add-on target (using node-swift)
  • Adds a native.js export that uses that add-on

For now, kept the other as the default, since the add-on can't be cross-platform compiled until this is released

@karaggeorge
Copy link
Member Author

karaggeorge commented Nov 13, 2024

@sindresorhus Any idea why when I ran this with the macos-14 image I got errors for missing properties. They are marked with if #avalable(macOS 15) 🤔

Building with macos-15 worked, but failed the tests. I got those failures intermittently locally too, still not sure why they happen. It only happens for the CLI, I think something to do with the way I use async with the run loop. I'll debug a bit more tonight

I didn't have any such issues using the aperture.node bindings, but I couldn't get rid of the CLI since I can't build it cross-platform

Could we do something similar to Kap, run the build in two workers one for arm and one for intel, generate two .node files and require them conditionally at runtime?

Or is there value in keeping the CLI version in general

Package.swift Outdated
)
],
dependencies: [
.package(url: "https://github.com/wulkano/Aperture", from: "2.0.1"),
.package(url: "https://github.com/apple/swift-argument-parser", from: "1.3.1")
.package(url: "https://github.com/wulkano/Aperture", branch: "george/rewrite-in-screen-capture-kit"),
Copy link
Member Author

Choose a reason for hiding this comment

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

The branch here should change back to the real version when the Aperture PR is merged/released

@sindresorhus
Copy link
Contributor

Any idea why when I ran this with the macos-14 image I got errors for missing properties. They are marked with if #avalable(macOS 15) 🤔

Using new properties only works with a new Xcode, which may require the latest macOS image. #avalable(macOS 15) means it will be ignored when running on older macOS versions, but you still need latest Xcode with latest SDK to build it.

@sindresorhus
Copy link
Contributor

For now, kept the other as the default, since the add-on can't be cross-platform compiled until this is released

The goal should be to only have the native add-on. Doesn't make sense to maintain both. And the native add-on should be much faster and reliable.

@@ -3,30 +3,22 @@ import Aperture

struct Options: Decodable {
let destination: URL
let targetId: String?
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let targetId: String?
let targetID: String?

index.d.ts Outdated
Comment on lines 21 to 22
applicationName?: string;
applicationBundleIdentifier?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick (my preference only):

Suggested change
applicationName?: string;
applicationBundleIdentifier?: string;
appName?: string;
appBundleIdentifier?: string;

@karaggeorge
Copy link
Member Author

@sindresorhus I think this is ready for another review

I've managed to build a cross-platform add-on, although we have to use xcodebuild for now, until this is available

It means that we need this entire build directory included, otherwise it throws when imported:
Screenshot 2024-11-22 at 9 56 29 AM

I've set up GH actions to build/test in arm macos-15 and upload the add-on as an artifact, then download and run tests on an intel macos-13 image (the only free intel one). Currently I'm skipping the recording test since they have some issues with recording permissions that I doubt they'll fix for such an old image, but even just running the other ones verifies the add-on is compatible with intel

Changed the API of the package a bit to have separate entry-points for each target since they have different sets of options. It was either that, or a complex TS overload, figured this was better, but not a strong preference.

One thing I'm not sure about is do we need to do anything for ASAR with the add-on path like we did for the CLI? Or is that ok as is with:
const nativeModule = createRequire(import.meta.url)('./build/aperture.framework/Versions/A/aperture.node');

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants