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

add support for weapp #653

Closed
wants to merge 1 commit into from
Closed

add support for weapp #653

wants to merge 1 commit into from

Conversation

taoqf
Copy link
Contributor

@taoqf taoqf commented Jul 26, 2017

For now, if using with webpack, need wait for browserify/timers-browserify#24 and max-mapper/websocket-stream#129 merged.

@mcollina
Copy link
Member

I'm a bit lost in what is weapp, wx and such.

@taoqf
Copy link
Contributor Author

taoqf commented Jul 27, 2017

https://mp.weixin.qq.com/cgi-bin/wx
I have added something to the readme,but tests could not be added because it's only could run in container.

@codecov
Copy link

codecov bot commented Jul 27, 2017

Codecov Report

Merging #653 into master will decrease coverage by 3.48%.
The diff coverage is 79.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #653      +/-   ##
==========================================
- Coverage   93.22%   89.73%   -3.49%     
==========================================
  Files           8        9       +1     
  Lines         664      760      +96     
  Branches      160      198      +38     
==========================================
+ Hits          619      682      +63     
- Misses         45       78      +33
Impacted Files Coverage Δ
lib/connect/wx.js 55.38% <55.38%> (ø)
lib/connect/index.js 94.17% <94.11%> (-3.05%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5afa5ac...83e8037. Read the comment docs.

@taoqf
Copy link
Contributor Author

taoqf commented Jul 27, 2017

@mcollina What can I do with the failled checks?

@taoqf
Copy link
Contributor Author

taoqf commented Jul 28, 2017

@mcollina Weixin(wx) is an application runs on android, iPhone, you may think it as cordova, It allow h5 application(we call it weapp sometime) runs in it, and it allow users to use WebSocket using it's api.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

I'm not landing code without unit test. So, we should change the approach:

how about we add support so that you can specificy a custom protocol? That would probably help.

Also, why lib/connect/index.js look all rewritten? Maybe spacing?

@taoqf
Copy link
Contributor Author

taoqf commented Jul 31, 2017

Then could I add some unit test with failing judge? I think there are so many chinese programmer would need this.

@@ -17,6 +17,5 @@
"mocha": true,
"indent": 2,
"latedef": true,
"immed": true,
Copy link
Member

Choose a reason for hiding this comment

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

why this?

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 was defined multi times. see line 4

@mcollina
Copy link
Member

Unit tests will be good?

@taoqf
Copy link
Contributor Author

taoqf commented Jul 31, 2017

I don't know how to add unit test yet, but I would try my best.

@taoqf
Copy link
Contributor Author

taoqf commented Jul 31, 2017

I have done some test, not unit test, it works good.

@taoqf
Copy link
Contributor Author

taoqf commented Jul 31, 2017

@mcollina so need I add unit test for this pr?

@mcollina
Copy link
Member

Yes.

@taoqf
Copy link
Contributor Author

taoqf commented Aug 1, 2017

Hi, @mcollina , I have add some test into /test/wx.js and passed npm run weapp-test.
is this the right way to add unit test?

@taoqf
Copy link
Contributor Author

taoqf commented Aug 1, 2017

Whatever I try, still could pass codecov check. any help?

@mcollina
Copy link
Member

mcollina commented Aug 1, 2017

I'll merge this. However, can you please restore lib/connect/index.js to what is there on master? It's all reported as changed in this PR.

}

protocols.wx = require('./wx')
protocols.wxs = require('./wx')
Copy link
Member

Choose a reason for hiding this comment

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

we should not be adding wx if it's not a browser. the main wx object is not there.

@taoqf
Copy link
Contributor Author

taoqf commented Aug 2, 2017

@mcollina , Thanks, I move the code into else block, and I skiped the unit tests I added because it could not be running on node.js.

@mcollina
Copy link
Member

mcollina commented Aug 2, 2017

You can remove the skipped tests.

What have you changed to the lib/connect/index.js file? GH reports it was all changed, but it does not seems so. Can you please restore it to what was before and apply only your modifications?

@taoqf
Copy link
Contributor Author

taoqf commented Aug 3, 2017

I tried not skip the tests and it will cause a failing test after that test.

189 passing (18s)
  1 failing

  1) MqttSecureClient pinging should reconnect if pingresp is not sent:
     Uncaught Error: connect ECONNREFUSED 127.0.0.1:8883
      at Object.exports._errnoException (util.js:1024:11)
      at exports._exceptionWithHostPort (util.js:1047:20)
      at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1150:14)

I don't really know what was wrong with github, I had not changed the whole file connect/index.js.
I will try to create a new pr later and see if gh reports the same.

@taoqf taoqf closed this Aug 3, 2017
@taoqf taoqf mentioned this pull request Aug 3, 2017
@vikingsailor
Copy link

@taoqf 在微信小程序里怎么使用mqtt连接AZURE?我已经在nodejs中联通了iot云,但是想在小程序中看到各设备的消息?如何实现?

@taoqf
Copy link
Contributor Author

taoqf commented Sep 4, 2017

Could you please describe azure?

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.

3 participants