Sdk browser #236
Sdk browser #236
Conversation
|
Some strange error with browser tests so far, looks like |
|
Hm, started seeing it locally as well... |
|
That's killing me, just ran locally and works fine |
|
In addition AppVeyor tests failed because of HTTP timeout |
|
So I've fixed the browser env issue, but still the AppVeyor tests fail because of HTTP timeout. Any idea how to deal with it? Just try increasing the timeout in tests definition? Linux/macOS tests don't seem to suffer from it and the full suite runs on my machine. |
|
Yay, Travis passed. The timeout is already 30 minutes but it's the test timeout, I think there might be a separate |
| fs = require('fs') | ||
| path = require('path') | ||
|
|
||
| exports.loadEnv = -> |
There was a problem hiding this comment.
Is it possible to replace this with https://www.npmjs.com/package/dotenv ?
There was a problem hiding this comment.
Yes, absolutely. I was aware of it but was lazy to properly look into the docs :)
| # }); | ||
| ### | ||
| exports.getAll = (callback) -> | ||
| Promise.try -> |
There was a problem hiding this comment.
This can probably be Promise.try(settings.getAll)
There was a problem hiding this comment.
I did not rewrite the majority of code, just reindented almost every module to make it a factory instead of the static export. But will still go through your comment (I wish the test suite took less than 20 minutes to run...)
There was a problem hiding this comment.
This specific one I would ignore for two reasons:
- uniformity with the rest of the code in this module
- not thinking about if the
settingsmodule methods are bound or not
There was a problem hiding this comment.
Yeah, I'm ok with leaving this one as-is, btw something that would be huge imo is if you can get the coffeescript headers added to the js files as I think that would cause github to exclude them from the diff as autogenerated files
|
|
||
| deps = { | ||
| settings, | ||
| request: getRequest(opts), |
There was a problem hiding this comment.
There's no need for the commas
| exports.create = (title, key, callback) -> | ||
|
|
||
| # Avoid ugly whitespaces | ||
| key = key.trim() |
There was a problem hiding this comment.
If key isn't a string this will throw, breaking the expectation that a promise will always be returned
| device: deviceModel.get(uuid) | ||
| pubNubKeys: configModel.getPubNubKeys() | ||
| .then (results) -> | ||
| return logs.subscribe(results.pubNubKeys, results.device) |
There was a problem hiding this comment.
I think it'd be nicer to do .then ({ pubNubKeys, device }) ->
| # }); | ||
| ### | ||
| exports.isPassed = (callback) -> | ||
| token.getProperty('twoFactorRequired').then (twoFactorRequired) -> |
There was a problem hiding this comment.
For the sake of the browser bundle size I try to use as few lodash as possible, so I will keep it as is.
|
CS headers, like "generated by CS vX"? Any link to confirm GH is smart
enough to exclude them in this case? Again I didn't configure the build
process here in the first place so will have to look into why they're not
added by default.
…On Wed, Dec 7, 2016 at 1:18 AM, Page- ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/settings.coffee <#236>:
> + # @Fulfil {Object} - settings
+ # @returns {Promise}
+ #
+ # @example
+ # resin.settings.getAll().then(function(settings) {
+ # console.log(settings);
+ # });
+ #
+ # @example
+ # resin.settings.getAll(function(error, settings) {
+ # if (error) throw error;
+ # console.log(settings);
+ # });
+ ###
+ exports.getAll = (callback) ->
+ Promise.try ->
Yeah, I'm ok with leaving this one as-is, btw something that would be huge
imo is if you can get the coffeescript headers added to the js files as I
think that would cause github to exclude them from the diff as
autogenerated files
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#236>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAgGCBVg-ktF8ZC8pKPoJ99GlfvpZRUqks5rFd9BgaJpZM4K79lb>
.
--
Eugene Mirotin
Senior Frontend Engineer
site: Resin.io <https://resin.io/>, twitter: @resin_io
<https://twitter.com/resin_io>
|
And the code that actually does the detection for coffeescript: https://github.com/github/linguist/blob/master/lib/linguist/generated.rb#L139-L176 |
|
Cool, thanks a lot. Will check how to return the CS standard comment.
…On Wed, Dec 7, 2016 at 7:01 AM, Page- ***@***.***> wrote:
https://github.com/github/linguist#linguist
This library is used on GitHub.com to detect blob languages, ignore binary
or vendored files, suppress generated files in diffs, and generate language
breakdown graphs.
https://github.com/github/linguist#generated-file-detection
Not all plain text files are true source files. Generated files like
minified js and compiled CoffeeScript can be detected and excluded from
language stats. As an added bonus, unlike vendored and documentation files,
these files are suppressed in diffs.
And the code that actually does the detection for coffeescript:
https://github.com/github/linguist/blob/master/lib/
linguist/generated.rb#L139-L176
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#236 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAgGCF4-D2sNkqHTrOY9yvDZ2VD_sEnMks5rFi-igaJpZM4K79lb>
.
--
Eugene Mirotin
Senior Frontend Engineer
site: Resin.io <https://resin.io/>, twitter: @resin_io
<https://twitter.com/resin_io>
|
|
Hey @pimterry this is an important PR for the changes planned to a bunch of our interfaces. Other comments are welcome as well, of course, but for your convenience here's the overview:
|
| javascript: 'build/**/*.js' | ||
| directories: | ||
| doc: 'doc/' | ||
| build: 'build/' | ||
|
|
||
| gulp.task 'coffee', -> | ||
| gulp.src(OPTIONS.files.app) | ||
| .pipe(coffee()).on('error', gutil.log) | ||
| .pipe(coffee(header: true)).on('error', gutil.log) |
There was a problem hiding this comment.
@jviotti it'd be awesome to gradually add this to our other modules as well (as changes are made to them), it lets the .js files get suppressed in the diff and is overall so much easier to review :D
|
You've committed a |
|
Yes and it's how you should do that, read more here:
https://yarnpkg.com/en/docs/yarn-lock
https://yarnpkg.com/en/docs/migrating-from-npm
TLDR: clients that have yarn (like travis and any user who has it) will
have exactly the same set of deps I have had when running the tests on my
machine. Others will still work with npm as before. It's like
npm-shrinkwrap but faster and deterministic.
…On Thu, Dec 8, 2016 at 11:40 PM, Page- ***@***.***> wrote:
You've committed a yarn.lock file btw @emirotin
<https://github.com/emirotin>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#236 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAgGCGpvnITGFmntJdFn_qJI7Ev2lBe4ks5rGGtRgaJpZM4K79lb>
.
--
Eugene Mirotin
Senior Frontend Engineer
site: Resin.io <https://resin.io/>, twitter: @resin_io
<https://twitter.com/resin_io>
|
|
Iff we start using lerna that will be as easy as doing nothing special as
it will be in the shared config. Actually this change is something that
reminded me about the idea of monorepo.
…On Thu, Dec 8, 2016 at 11:34 PM, Page- ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In gulpfile.coffee
<#236 (review)>
:
> javascript: 'build/**/*.js'
directories:
doc: 'doc/'
build: 'build/'
gulp.task 'coffee', ->
gulp.src(OPTIONS.files.app)
- .pipe(coffee()).on('error', gutil.log)
+ .pipe(coffee(header: true)).on('error', gutil.log)
@jviotti <https://github.com/jviotti> it'd be awesome to gradually add
this to our other modules as well (as changes are made to them), it lets
the .js files get suppressed in the diff and is overall so much easier to
review :D
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#236 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAgGCLyC30DkjqB5TMkyRlJqrBIfy_mPks5rGGnvgaJpZM4K79lb>
.
--
Eugene Mirotin
Senior Frontend Engineer
site: Resin.io <https://resin.io/>, twitter: @resin_io
<https://twitter.com/resin_io>
|
|
@emirotin: Sure, I can take a look at this. Not immediately though, as I don't actually work for Resin yet - I've just been keeping an occasional eye on interesting things since the summit. I start properly on Monday. Once I'm all set up with the basics then I'll take a proper look at this. |
|
Sure, thanks.
|
|
I've got this all set up and working locally, and had a reasonable look at the changes, but I can't see any specific issues that would break this. I think it's pretty likely that the timeouts that are breaking these tests aren't from this change, and they're part of a wider existing issue instead. The failures here seem to be a variable mix of ETIMEDOUT, which don't match between the two identical builds. This has been happening for months on and off for a while on all sorts of other Resin-SDK builds too: Doesn't look like the builds for this repo have been reliably passing since July: https://ci.appveyor.com/project/resin-io/resin-sdk/history. My best guess for why this is only failing on Windows and not on Travis's linux builds is because the built-in connect timeout is much stricter on Windows (see nodejs/node-v0.x-archive#2324). Tricky to confirm this though, and it doesn't look like it's possible to do the easy short-term fix of boosting the connect timeout beyond the OS default (you can only shorten it). This does suggest that the API endpoints are intermittently really slow (occasionally up to 12 seconds to get a response), which I can't easily reproduce locally. Do we have any stats or monitoring for those? Is that plausible? Alternatively something else could have been causing intermittent timeouts for months, but that's pretty surprising too. I'm going to keep looking into this for now as it needs fixing either way, but it might not need to block this PR - it doesn't make it any worse than it already is. If you re-run the builds a couple of times they'll probably come around to green again 😭 |
|
Hey @pimterry thanks for the great investigation! @Page- did I address your review? |
|
I'm glad you resolved it! Looks good to me! |
|
So the yarn.lock introduces a shrinkwrap but only for certain people and tests? I think I'd rather have the tests run against what will be installed when a user npm installs as that's the most common case and I'd want to make sure that works. If we want to switch to shrinkwrapping, then I'd like it for it to always be shrinkwrapped rather than only sometimes |
|
It provides guarantees to whom is using yarn, and doesn't change anything
to the others. I don't think it's bad.
Speaking about tests they guarantee nothing wrt the dependencies stability
as you run the tests today and the user may install the package in 5 days
with several dependencies already updated.
So it not worse as well wrt this.
…On Tue, Dec 13, 2016 at 4:09 AM, Page- ***@***.***> wrote:
So the yarn.lock introduces a shrinkwrap but only for certain people and
tests? I think I'd rather have the tests run against what will be installed
when a user npm installs as that's the most common case and I'd want to
make sure that works. If we want to switch to shrinkwrapping, then I'd like
it for it to always be shrinkwrapped rather than only sometimes
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#236 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAgGCC-PPjLmh-cL_9-oZ259Jx6XENyRks5rHfBCgaJpZM4K79lb>
.
--
Eugene Mirotin
Senior Frontend Engineer
site: Resin.io <https://resin.io/>, twitter: @resin_io
<https://twitter.com/resin_io>
|
switch to the factory pattern allow loading test env from the .env file lazy-load all the models update to mocha v3 fix deprecation warnings granular lodash imports use a more modern .asCallback name add yarn.lock use /v2/ api for tests add CS preamble
|
@jviotti @pimterry what do you think about adding the support for yarn? |
emirotin
force-pushed
the
sdk-browser
branch
from
fb3bdb5 to
0c0f964
Compare
|
Sounds like we should just try yarn and see if we have any problems.
I'm going to merge this PR as soon as all the tests pass. |
|
Looks like good to go?:) |
|
As we have some good convo WRT using a better |
…sh on windows again
|
With all the recent changes finally finished up and #245 merging them in, I think this really is good to go now. Is there anything else left over, or can we merge this @emirotin? I think immediate next steps are:
I'll get started on those shortly. Let me know if there's anything else related to this I should check out. |
|
Before merging I'd like to do several things:
Things to do before or after the release:
|
This now uses the new injection dependency approach, so we build each of these libraries and glue them together, rather than them internally instantiating their own local versions of one another.
|
Looks great.
Other than that let's merge as soon as the tests pass (unless other guys have any objections, of course). |
One bug solved en route here, highlighted by minification: we depended on the function name of errors thrown by Resin-Request, which isn't dependable (e.g. in prescence of minification). We now depend on .code instead, which should work reliably.
Build minified and unminified bundles
|
I've re-run the one last failed Travis job, just to be sure. Once I've got that passing, based on @emirotin's last comment not getting any objections, I'm going to merge this 🎉. Last chance to shout now if you disagree! |
|
🎉🎉🎉 |
|
Wow! Would you mind releasing this as 6.0.0-beta1?
I'd like to indicate that it has some major refactorings
…On Wed, Jan 11, 2017 at 2:35 PM, Tim Perry ***@***.***> wrote:
Merged #236 <#236>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#236 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAgGCBe5_miAL1XJpVQVB9SMUUFn7Nxvks5rRL6cgaJpZM4K79lb>
.
--
Eugene Mirotin
Senior Frontend Engineer
site: Resin.io <https://resin.io/>, twitter: @resin_io
<https://twitter.com/resin_io>
|
|
@emirotin Sure, done. I'm going to get started on trying to pull this into resin-cli now, we'll see how it goes. |
|
Ah, not done - |
|
I had a PR for that ready some time ago but it may need some adjustments.
Let me just push it and you can use it as a base or discard.
…On Wed, Jan 11, 2017 at 3:55 PM, Tim Perry ***@***.***> wrote:
@emirotin <https://github.com/emirotin> Sure, done. I'm going to get
started on trying to pull this into resin-cli now, we'll see how it goes.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#236 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAgGCDvWWB4GvQPgRY0OUoyD-p5gdGFvks5rRNE3gaJpZM4K79lb>
.
--
Eugene Mirotin
Senior Frontend Engineer
site: Resin.io <https://resin.io/>, twitter: @resin_io
<https://twitter.com/resin_io>
|
Same as before but from a local branch