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

Configure for v0.12 and io.js compatibility #571

Merged
merged 1 commit into from
Feb 17, 2015
Merged

Configure for v0.12 and io.js compatibility #571

merged 1 commit into from
Feb 17, 2015

Conversation

andrewbranch
Copy link
Contributor

Fixes #550. Sorry about the end-of-line spacing changes, my editor seemed to do that automatically for some reason.

@mdhooge
Copy link
Contributor

mdhooge commented Feb 16, 2015

It is no surprise, but this also solves the problem with node 0.12.0.
However, I wonder if this wouldn't be better to explicitly check for "object" instead of an inequality with "string". I could try myself, but right now, I'm back to v0.10.36 ;-)

@herom
Copy link
Contributor

herom commented Feb 16, 2015

Thanks a lot for your contribution @andrewbranch - we appreciate it a lot! This really should make it into a new release as soon as possible 👍

Before we can merge this, I'll have to ask you to revert all unnecessary changes (like the end-of-line changes, etc.) so that we can keep our Git History clean and don't clutter it with changes which shouldn't be in history (the history should show only real code changes, nothing more).

Also, I guess it would be of further value, if you could add a line in our .travis.yml file to include node v0.12.0 for testing along with your commit. If you do so, please amend and force push, or squash and force push your changes as we request 1 commit per PR as stated in our Guidelines for Submitting a Pull Request.

@andrewbranch
Copy link
Contributor Author

@mdhooge — I agree; changed to object.
@herom — I’m not sure how to overwrite commits I already pushed to GitHub. You can see that what I just tried kind of made things messier—my apologies. Would it be easiest to delete this PR and start a fresh one? Sorry for the inconvenience.

@andrewbranch
Copy link
Contributor Author

Fixed the commit history, but it looks like there are other issues arising with newer node versions. Working on adding fixes to the pull.

@andrewbranch
Copy link
Contributor Author

Most of the failing tests from the last build were caused by server.address().address() returning ::. I have no idea what that’s about, but I checked for it and replaced with 127.0.0.1. However, there are still two failing tests that I don’t know how to fix. Would appreciate help from someone more familiar with the project.

@robertjneal
Copy link
Contributor

It looks like you can fix the error by copying the conditional you used in server-test.js to the beforeEach of ssl-test.js, namely:
if (test.server.address().address === '0.0.0.0' || test.server.address().address === '::') {

I didn't checkout your branch to confirm, but that looks to be the issue.

@andrewbranch
Copy link
Contributor Author

Everything looks good now! Thanks to @mdhooge, @herom, and @robertjneal for the input/guidance!

@andrewbranch andrewbranch changed the title Fix strict mode errors Fix strict mode errors for v0.12 and io.js compatibility Feb 17, 2015
@andrewbranch andrewbranch changed the title Fix strict mode errors for v0.12 and io.js compatibility Configure for v0.12 and io.js compatibility Feb 17, 2015
@@ -2,5 +2,7 @@ language: node_js
node_js:
- 0.8
- 0.10
- 0.12
- iojs
before_install:
- npm install -g npm@~1.4.6
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only an addition is shown here—the dashes are part of the YAML syntax itself. :)

Choose a reason for hiding this comment

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

On a tangential note, is there a specific reason to stay with [email protected]?
It's almost a year old and npm's current requirement of node >= 0.8 seems like a non-issue no?

Copy link
Contributor

Choose a reason for hiding this comment

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

@andrewbranch oh, sorry - I was totally shure that this was deleted 😸

@simonjosefsson we needed npm v1.4.6 in order to let the tests pass for node v0.8.0 too while pulling in some new libraries. This could probably make it into another PR where we should discuss about support for the (very, very) old node v0.8.0 in general.

herom added a commit that referenced this pull request Feb 17, 2015
Configure for v0.12 and io.js compatibility
@herom herom merged commit 3595bfa into vpulim:master Feb 17, 2015
@herom
Copy link
Contributor

herom commented Feb 17, 2015

Thanks a ton @andrewbranch 👯

@andrewbranch andrewbranch deleted the fix-strict-mode-errors branch February 17, 2015 19:58
@andrewbranch
Copy link
Contributor Author

@herom My pleasure—one step closer to using io.js on Heroku!

diarmaidm pushed a commit to diarmaidm/node-soap that referenced this pull request Feb 3, 2016
Configure for v0.12 and io.js compatibility
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.

5 participants