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

Fix x64 no expected browser bug #79

Closed

Conversation

lprintf
Copy link
Contributor

@lprintf lprintf commented Jul 18, 2021

#78

@@ -12,6 +12,13 @@ async function main () {
log.level(process.env.LOG_LEVEL as any)
}

if (process.env.SIMULATED) {
Copy link
Member

Choose a reason for hiding this comment

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

Where does this SIMULATED variable comes from?

.gitignore Outdated
@@ -61,6 +61,7 @@ t/
/dist/

*.memory-card.json
config/config.yaml
Copy link
Member

Choose a reason for hiding this comment

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

Could you please explain why you add this ignore, what's the problem if we does not include it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a config file for run this project(genarate the registration file or run bridge), containing some personal setting.

@@ -12,6 +12,13 @@ async function main () {
log.level(process.env.LOG_LEVEL as any)
}

if (process.env.SIMULATED) {
// XXX This is aim to use the chrome referred by /usr/bin/chromium-browser as dependency of wechaty.node_modules.puppeteer module.
Object.defineProperty(process, 'arch', {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's a good idea to modify process.arch in our code.

It looks like a workaround and we should find a solution instead.

@lprintf lprintf closed this Jul 18, 2021
@huan
Copy link
Member

huan commented Jul 18, 2021

Instead of closing a PR, we should keep it open and continue improving it by pushing new commits.

@lprintf lprintf reopened this Jul 18, 2021
林宇靖 added 2 commits July 18, 2021 10:23
@lprintf lprintf mentioned this pull request Jul 18, 2021
8 tasks
@huan huan closed this Jul 18, 2021
@huan huan reopened this Jul 18, 2021
Copy link
Contributor Author

@lprintf lprintf left a comment

Choose a reason for hiding this comment

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

Because wechaty-puppet-wechat have not supported users to set chrome/chromium path with environment variable now, I add some code in src/wechaty-manager.ts.

@huan
Copy link
Member

huan commented Jul 18, 2021

Good to see you using the WECHATY_PUPPET_PUPPETEER_ENDPOINT to pass the settings to the sub-system.

However, I believe this support would be better to be added to https://github.com/wechaty/wechaty-puppet-wechat/blob/6975f402960f0af34f8a39341444228a31a97594/src/puppet-wechat.ts#L123-L130 because there is the right place to handle this feature.

It would be great if you can move this change to the above constructor.

@lprintf
Copy link
Contributor Author

lprintf commented Jul 19, 2021

@lprintf lprintf closed this Jul 19, 2021
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