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

updated native messaging host example #36626

Closed
wants to merge 0 commits into from

Conversation

salmin89
Copy link

@salmin89 salmin89 commented Nov 2, 2024

Description

The current host example does not work on MacOS. Mac does not have /proc/PID/fd/1

[Error: ENOENT: no such file or directory, open '/proc/22290/fd/1'] {
errno: -2,
code: 'ENOENT',
syscall: 'open',
path: '/proc/22290/fd/1'
}

Motivation

Replaced the example with a working one.

Additional details

Readers would be able to use this example. It has been tested against Firefox and Chromium based browsers.
Author repo: https://github.com/guest271314/NativeMessagingHosts/blob/main/nm_nodejs.js

Related issues and pull requests

Fixes #36513

@salmin89 salmin89 requested a review from a team as a code owner November 2, 2024 22:35
@salmin89 salmin89 requested review from willdurand and removed request for a team November 2, 2024 22:35
@github-actions github-actions bot added Content:WebExt WebExtensions docs size/m [PR only] 51-500 LoC changed labels Nov 2, 2024
const message = await readFullAsync(header[0]);
return message;
const buffer = new ArrayBuffer(0, {
maxByteLength: 1024 ** 2
Copy link
Contributor

Choose a reason for hiding this comment

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

[mdn-linter] reported by reviewdog 🐶

Suggested change
maxByteLength: 1024 ** 2
maxByteLength: 1024 ** 2,

const writable = new WritableStream({
write(value) {
process.stdout.write(value);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

[mdn-linter] reported by reviewdog 🐶

Suggested change
}
},

Comment on lines 258 to 263
const {
exit
} = process;
const {
argv: args
} = process;
Copy link
Contributor

Choose a reason for hiding this comment

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

[mdn-linter] reported by reviewdog 🐶

Suggested change
const {
exit
} = process;
const {
argv: args
} = process;
const { exit } = process;
const { argv: args } = process;

Comment on lines 297 to 299
new Uint8Array(new Uint32Array([message.length]).buffer),
message,
])
Copy link
Contributor

Choose a reason for hiding this comment

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

[mdn-linter] reported by reviewdog 🐶

Suggested change
new Uint8Array(new Uint32Array([message.length]).buffer),
message,
])
new Uint8Array(new Uint32Array([message.length]).buffer),
message,
])

])
.stream()
.pipeTo(writable, {
preventClose: true
Copy link
Contributor

Choose a reason for hiding this comment

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

[mdn-linter] reported by reviewdog 🐶

Suggested change
preventClose: true
preventClose: true,

@Josh-Cena
Copy link
Member

Could you make minimal changes required to make it work, rather than copying entire source code over?

guest271314 added a commit to guest271314/NativeMessagingHosts that referenced this pull request Nov 3, 2024
Copy link
Member

@Rob--W Rob--W left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR!

While it is true that procFS is not supported on macOS, and that alternatives should be used, this PR includes many other changes that are not strictly required. Could you modify the PR, to only include the strictly necessary changes? That is basically replacing the use of /proc/pid/fd/1 with process.stdout.

@salmin89
Copy link
Author

salmin89 commented Dec 3, 2024

Thanks for opening this PR!

While it is true that procFS is not supported on macOS, and that alternatives should be used, this PR includes many other changes that are not strictly required. Could you modify the PR, to only include the strictly necessary changes? That is basically replacing the use of /proc/pid/fd/1 with process.stdout.

I updated the PR to use process.stdout instead :)

Copy link
Member

@Rob--W Rob--W left a comment

Choose a reason for hiding this comment

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

The PR still makes many unnecessary changes. Please take a look at the diff at https://github.com/mdn/content/pull/36626/files

There are many more changes than strictly necessary.

@salmin89 salmin89 closed this Dec 3, 2024
@github-actions github-actions bot added size/xs [PR only] 0-5 LoC changed and removed size/m [PR only] 51-500 LoC changed labels Dec 3, 2024
@salmin89
Copy link
Author

salmin89 commented Dec 3, 2024

The PR still makes many unnecessary changes. Please take a look at the diff at https://github.com/mdn/content/pull/36626/files

There are many more changes than strictly necessary.

my fork was outdated and I used revert before syncing so I don't know what happened. I re-forked and opened a new PR to keep the change minimal:
#37067

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebExt WebExtensions docs size/xs [PR only] 0-5 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nm_nodejs.mjs example is not running on firefox linux
3 participants