fix: conditional binding (#23) #24

mmaietta · 2021-03-13T02:44:52Z

Enhancement and fixes: #23 (Module did not self-register)
#23 occurs because of Linux having no export defined due to having no sources

node-mac-permissions/binding.gyp

Lines 4 to 10 in c74d338

    
           "sources": [ ], 
        
           "conditions": [ 
        
             ['OS=="mac"', { 
        
               "sources": [ 
        
                 "permissions.mm" 
        
               ], 
        
             }]

Adding conditional binding so that we can retain type-safety of the import for things like permissions.AuthType as function params with a generic import that is platform-agnostic.

import * as permissions from 'node-mac-permissions'
async requestSystemAccessPermission (accessType: permissions.AuthType): Promise<permissions.PermissionType> { }

Adding an additional test to run on a separate Linux node for this use case

Opening as DRAFT because I'm unable to actually test this on Linux, only verified through Github actions Ubuntu node and through headless docker container (via xvfb-run). Going to test on windows soon. Wanted to get a head start on any feedback though to make sure I'm not already off the rails

…mport for things like `permissions.AuthType` as function params with a generic import that is platform-agnostic. Enhancement and fixes: codebytere#23 Adding additional test to run on a separate linux node

mmaietta · 2021-03-13T02:53:09Z

@codebytere it seems that xcode 12.4 changed some things and now it throws this error when attempting to build:

warning: /Applications/Xcode_12.4.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool: archive library: Release/nothing.a the table of contents is empty (no object file members in the library define global symbols)
  CXX(target) Release/obj.target/permissions/permissions.o
../permissions.mm:274:8: error: duplicate case value: 'kCLAuthorizationStatusAuthorized' and 'kCLAuthorizationStatusAuthorizedAlways' both equal '3'
  case kCLAuthorizationStatusAuthorizedAlways:
       ^
../permissions.mm:273:8: note: previous case defined here
  case kCLAuthorizationStatusAuthorized:
       ^
1 error generated.
make: *** [Release/obj.target/permissions/permissions.o] Error 1

I've commented out that line since they're duplicate values and left an additional comment on the new behavior.

node-mac-permissions/permissions.mm

Lines 273 to 274 in 321a1f8

    
           // Xcode 12.4+ : duplicate case value: 'kCLAuthorizationStatusAuthorized' and 'kCLAuthorizationStatusAuthorizedAlways' both equal '3' 
        
           // case kCLAuthorizationStatusAuthorizedAlways:

codebytere · 2021-03-13T12:50:54Z

@mmaietta if you rebase it should be good now

.github/workflows/test.yml

index.js

test/module.spec.js

codebytere · 2021-03-13T12:59:32Z

index.js

@@ -34,16 +51,7 @@ function askForFoldersAccess(folder) {
 }

 module.exports = {
-  askForCalendarAccess: permissions.askForCalendarAccess,
-  askForContactsAccess: permissions.askForContactsAccess,
+  ...permissions,


we probably also need to handle calls to askForFoldersAccess and getAuthStatus - permissions.askForFoldersAccess.call(this, folder) @ L33 for example probably blows up, does it not?

These should also be covered in the additional unit test that runs on Linux 🙂

node-mac-permissions/test/module.spec.js

Lines 67 to 68 in 321a1f8

expect(permissions.getAuthStatus('contacts')).to.be.undefined

expect(permissions.askForFoldersAccess('desktop')).to.be.undefined

The type/folder is validated, and the call is still to permissions.askForFoldersAccess: () => undefined from the original stub, so it behaves as expected with the test's assert. I originally had nonMacResponse = (...args) => undefined but then ...args was just flagged as unused.

These two tests also run on all nodes to enforce consistency of the arg cross-platform.

node-mac-permissions/test/module.spec.js

Lines 10 to 14 in 321a1f8

it('should throw on invalid types', () => {

expect(() => {

permissions.getAuthStatus('bad-type')

}).to.throw(/bad-type is not a valid type/)

})

node-mac-permissions/test/module.spec.js

Lines 40 to 44 in 321a1f8

it('should throw on invalid types', () => {

expect(() => {

permissions.askForFoldersAccess('bad-type')

}).to.throw(/bad-type is not a valid protected folder/)

})

The only notable difference is that the index.d.ts does not specify the new undefined behavior. I felt that was acceptable as this is explicitly a mac-only module, this enhancement is mainly to allow type-safety with a standard import in local projects.

package.json

…mport for things like `permissions.AuthType` as function params with a generic import that is platform-agnostic. Enhancement and fixes: codebytere#23 Adding additional test to run on a separate linux node

# Conflicts: # README.md # index.js # package.json # test/module.spec.js

mmaietta · 2021-06-01T16:53:56Z

Marking as ready for review.

My team and I have already been leveraging this implementation via patch-package.
We're already running with this fix in our prod release.

Mike Maietta added 2 commits March 12, 2021 18:27

Adding conditional binding so that we can retain type-safety of the i…

95ffae0

…mport for things like `permissions.AuthType` as function params with a generic import that is platform-agnostic. Enhancement and fixes: codebytere#23 Adding additional test to run on a separate linux node

Updating deprecated config var

c53bda2

codebytere reviewed Mar 13, 2021

View reviewed changes

mmaietta force-pushed the misc/conditional-binding branch from 321a1f8 to 92e80cd Compare March 14, 2021 16:50

Adding conditional binding so that we can retain type-safety of the i…

b1d9399

…mport for things like `permissions.AuthType` as function params with a generic import that is platform-agnostic. Enhancement and fixes: codebytere#23 Adding additional test to run on a separate linux node

mmaietta force-pushed the misc/conditional-binding branch from 92e80cd to b1d9399 Compare March 14, 2021 17:19

mmaietta and others added 2 commits April 1, 2021 19:57

Merge branch 'master' into misc/conditional-binding

f9983f4

Merge branch 'master' into misc/conditional-binding

cd3ae41

# Conflicts: # README.md # index.js # package.json # test/module.spec.js

mmaietta marked this pull request as ready for review June 1, 2021 16:50

Merge branch 'master' into misc/conditional-binding

371a44c

mmaietta mentioned this pull request Sep 10, 2021

chore(deps): specify operating system #27

Closed

codebytere deleted the branch codebytere:master November 11, 2021 18:14

codebytere closed this Nov 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: conditional binding (#23) #24

fix: conditional binding (#23) #24

mmaietta commented Mar 13, 2021

mmaietta commented Mar 13, 2021 •

edited

Loading

codebytere commented Mar 13, 2021

codebytere Mar 13, 2021

mmaietta Mar 13, 2021

mmaietta commented Jun 1, 2021 •

edited

Loading

	"sources": [ ],
	"conditions": [
	['OS=="mac"', {
	"sources": [
	"permissions.mm"
	],
	}]

	expect(permissions.getAuthStatus('contacts')).to.be.undefined
	expect(permissions.askForFoldersAccess('desktop')).to.be.undefined

	it('should throw on invalid types', () => {
	expect(() => {
	permissions.getAuthStatus('bad-type')
	}).to.throw(/bad-type is not a valid type/)
	})

	it('should throw on invalid types', () => {
	expect(() => {
	permissions.askForFoldersAccess('bad-type')
	}).to.throw(/bad-type is not a valid protected folder/)
	})

fix: conditional binding (#23) #24

fix: conditional binding (#23) #24

Conversation

mmaietta commented Mar 13, 2021

mmaietta commented Mar 13, 2021 • edited Loading

codebytere commented Mar 13, 2021

codebytere Mar 13, 2021

Choose a reason for hiding this comment

mmaietta Mar 13, 2021

Choose a reason for hiding this comment

mmaietta commented Jun 1, 2021 • edited Loading

mmaietta commented Mar 13, 2021 •

edited

Loading

mmaietta commented Jun 1, 2021 •

edited

Loading