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 #923 #935

Merged
merged 2 commits into from
Feb 26, 2018
Merged

Fix #923 #935

merged 2 commits into from
Feb 26, 2018

Conversation

xinbenlv
Copy link
Contributor

@xinbenlv xinbenlv commented Oct 30, 2017

Fixes #923

@huan
Copy link
Member

huan commented Oct 30, 2017

Awesome! Could you please fix the CI linting error before we could merge it?

https://travis-ci.org/Chatie/wechaty/jobs/294637040#L650

@huan
Copy link
Member

huan commented Nov 6, 2017

ping @xinbenlv Please fix the linting error so that we could merge it, thanks! :)

Copy link
Member

@lijiarui lijiarui left a comment

Choose a reason for hiding this comment

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

Change wechaty function to the latest one.

bot.start()
.catch(e => {
log.error('Bot', 'init() fail: %s', e)
bot.quit()
Copy link
Member

Choose a reason for hiding this comment

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

The latest wechaty function is stop(), maybe we should change the latest one instead.

@@ -444,7 +444,7 @@ export class Wechaty extends EventEmitter implements Sayable {
/**
* Quit the bot
*
* @deprecated
* @deprecated use stop() instead
* @returns {Promise<void>}
* @example
* await bot.quit()
Copy link
Member

Choose a reason for hiding this comment

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

await bot.stop()

finis((code, signal) => {
const exitMsg = `Wechaty exit ${code} because of ${signal} `
console.log(exitMsg)
bot.say(exitMsg)
Copy link
Member

@lijiarui lijiarui Nov 20, 2017

Choose a reason for hiding this comment

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

maybe it should add bot.stop() and process.exit(-1)here, or chrome will restart again and again... and node cannot be quit.... see #978

@huan
Copy link
Member

huan commented Nov 23, 2017

ping @xinbenlv

@xinbenlv
Copy link
Contributor Author

xinbenlv commented Nov 23, 2017 via email

@xinbenlv
Copy link
Contributor Author

xinbenlv commented Nov 23, 2017 via email

@huan
Copy link
Member

huan commented Nov 23, 2017

I did not use hotImport lot but it always works fine with me.

In order to import a class, you can have a look into https://github.com/zixia/hot-import/blob/bc380c8238d3f7e1d410fd21d5e404de4aa8760e/src/hot-import.spec.ts#L82

The unit tests & fixtures would help you understand the inside logic, and please file issue if you meet any bug.

Thanks.

@suntong
Copy link
Contributor

suntong commented Feb 26, 2018

@xinbenlv

I am using this feature (hot-import) and start to experience some problem...

do you have more updates on that please?

Have you tried https://github.com/Chatie/wechaty/tree/master/example/hot-reload-bot?

@zixia, what's the difference between the hot-reload-bot demo and hot-import?

Thanks

huan added a commit that referenced this pull request Feb 26, 2018
@huan
Copy link
Member

huan commented Feb 26, 2018

@suntong I had just written a new example of how to use hot-import feature which natively supported by Wechaty at here: https://github.com/Chatie/wechaty/tree/master/example/hot-import-bot

Hope that could help you.

@xinbenlv Please have a look at it too, I tested it and felt it works well. thanks.

Good luck!

@huan huan merged commit 876cba7 into wechaty:master Feb 26, 2018
@suntong
Copy link
Contributor

suntong commented Feb 26, 2018

ha, I see your new changes right in front of my eyes, from your own to @xinbenlv's PR. :-)

So, @xinbenlv & @zixia, I looked at the

https://github.com/Chatie/wechaty/blob/master/example/hot-import-bot/index.js and
https://github.com/Chatie/wechaty/blob/master/example/hot-import-bot/listeners/on-message.js

but can't find where hot-import is utilized. can you point out to me where please?

@suntong
Copy link
Contributor

suntong commented Feb 26, 2018

Oh, it's now completely embedded into the core code so no external fiddling necessary. Cool.

Ok, then, how to make use of the feature to load config files? Thx.

@huan
Copy link
Member

huan commented Feb 26, 2018

@suntong Sorry it's only the hot-import for the module is supported by the core code.

You still have to do load config files job by yourself.

@suntong
Copy link
Contributor

suntong commented Feb 26, 2018

OK. I'll try hot-importing them myself via the API... thx.

@suntong
Copy link
Contributor

suntong commented Mar 8, 2018

@xinbenlv,

How do you start this demo, apart from running from docker?

I tried both node index.js and ts-node index.js but the results are the same:

20:15:06 ERR HotImport importFile(/.../wechaty/example/hot-import-bot/listeners/on-friend.js) rejected: SyntaxError: Unexpected token export
20:15:06 ERR HotImport importFile(/.../wechaty/example/hot-import-bot/listeners/on-login.js) rejected: SyntaxError: Unexpected token export
...
export default async function onFriend (contact, request) {
^^^^^^
SyntaxError: Unexpected token export
...

thanks!

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.

4 participants