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

Update dependency and add missing istanbul dependency #47

Merged
merged 2 commits into from Sep 7, 2015
Merged

Conversation

mohsen1
Copy link
Contributor

@mohsen1 mohsen1 commented Sep 6, 2015

Also fix a test that was breaking due to swagger-tools change and clean up travis.yaml file

@mohsen1
Copy link
Contributor Author

mohsen1 commented Sep 6, 2015

@IvanGoncharov please review

@@ -1,7 +1,9 @@
sudo: false
language: node_js
script:
- npm test && npm run coveralls
- npm test
- npm run coveralls
Copy link
Collaborator

Choose a reason for hiding this comment

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

From Travis docs:

When one of the build commands returns a non-zero exit code, the Travis CI build runs the subsequent commands as well, and accumulates the build result.

http://docs.travis-ci.com/user/customizing-the-build/#Customizing-the-Build-Step

So if npm test will fail Travis will try to execute npm run coveralls and it's produce more errors which will make error diagnostic harder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's okay.

@mohsen1
Copy link
Contributor Author

mohsen1 commented Sep 7, 2015

I just confirmed that it's iojs' new URL module breaking changes that is causing the failure. I'm digging in to see if it's iojs' bug or not.

@IvanGoncharov
Copy link
Collaborator

Ok. I'm currently working on feature that will extend API, we discussed it on Gitter.
I think it better to push it in scope of 1.0.0.
I will submit PR tomorrow.

@mohsen1
Copy link
Contributor Author

mohsen1 commented Sep 7, 2015

Yup! It's iojs' breaking change. Found it here. I believe iojs is doing it right. I'm trying to see if using URL package here fixes the issue in all VMs?

@IvanGoncharov
Copy link
Collaborator

IMHO, its better to use https://github.com/medialize/URI.js
It simplify a code and remove workarounds like this.
I can make PR for this.

Also fix a test that was breaking due to swagger-tools change and clean up travis.yaml file

This also copies over url.js file from nodejs/node to fix compability
issues
@mohsen1
Copy link
Contributor Author

mohsen1 commented Sep 7, 2015

ok @IvanGoncharov that makes sense. Copying over code is never a good idea. I'll merge this. Please make your changes in master after this merge.

mohsen1 added a commit that referenced this pull request Sep 7, 2015
Update dependency and add missing istanbul dependency
@mohsen1 mohsen1 merged commit 43d9934 into master Sep 7, 2015
whitlockjc pushed a commit that referenced this pull request Aug 31, 2021
Update dependency and add missing istanbul dependency
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.

None yet

2 participants