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

Travis: enable testing of node v4 + --harmony #501

Merged
merged 11 commits into from
Jul 12, 2016
Merged

Travis: enable testing of node v4 + --harmony #501

merged 11 commits into from
Jul 12, 2016

Conversation

michaelgerakis
Copy link
Contributor

@wardpeet
Copy link
Collaborator

wardpeet commented Jul 9, 2016

Looks a bit hacky in my opinion.. Not sure what the rest thinks

@@ -18,9 +19,9 @@ before_script:
- sleep 5
script:
- npm run lint
- npm run unit
- if [[ $(node -v) =~ ^v4.* ]]; then npm run unit:harmony; else npm run unit; fi
Copy link
Member

Choose a reason for hiding this comment

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

Curious if we could do like a export isNode4=( $(node -v) =~ ^v4.*)
could then do [[ isNode4 ]] && npm run unit:harmony || npm run unit

would feel a little cleaner. :)

Copy link
Member

@paulirish paulirish Jul 11, 2016

Choose a reason for hiding this comment

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

hmmm actually....
i'm seeing a pattern here using matrix that seems a bit smarter: arrizalamin/js-function-reflector/.travis.yml (or 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.

That sounds good, except for the && and ||. I've personally refrained from using them because if npm run unit:harmony fails then npm run unit will be run, which is not what we want.

@michaelgerakis
Copy link
Contributor Author

Updated

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