-
Notifications
You must be signed in to change notification settings - Fork 3
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
chore(build): Add semantic-release in Travis #33
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments, but all in all this looks good.
Does this mean we are going to be "live"?
@@ -1,13 +1,20 @@ | |||
language: node_js | |||
node_js: | |||
- "node" | |||
- "10" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You said node
is set to 10
, do we need to add 9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
node
is set to the latest version. So for now it's 10
but when 11
comes out, it'll be that. The specific ones I call out are those that have long-term support (LTS) which are the evens
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I didn't know that about the LTS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why add 10 if node is already at 10?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because once newer versions come out, node
will no longer be 10. It's how I caught the issue with Node 10 so quickly. node
became 10 when it was released and broke the build. My thinking is that it'd be good to make sure that the SDK can always run on the latest version of Node
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So node
will auto catch 11 when it comes out? Or 12?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever the latest version is. So 11, then 12, etc...
- "8" | ||
- "6" | ||
|
||
# Cache dependencies in $HOME/.yarn-cache across builds | ||
cache: yarn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this no longer needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue I fixed with #32 had to do with the node_modules installed for Node 6/8 not working with Node 10. So we can no longer share dependencies across builds as a result because different dependencies get installed depending on Node version. I think it's safer this way...
Can't go live until we get control of the |
Wow, that was published 6 years ago. 😳 |
Yeah! @ryanjarvinen worked at EB way back when it was just API v1 😄. He built out the original developer portal. I'm impressed that we even had a node module back then. I didn't even know what NPM was 😂 |
README.md
Outdated
[![version](https://img.shields.io/npm/v/eventbrite.svg)](http://npm.im/eventbrite) | ||
[![downloads](https://img.shields.io/npm/dt/eventbrite.svg)](http://npm-stat.com/charts.html?package=eventbrite&from=2018-05-01) | ||
[![module formats: esm, cjs, & umd](https://img.shields.io/badge/module%20formats-esm%2C%20cjs%2C%20umd-green.svg)](https://unkpg.com/eventbrite/) | ||
[![semantic-release](https://img.shields.io/badge/%20%20%F0%9F%93%A6%F0%9F%9A%80-semantic--release-e10079.svg)](https://github.com/semantic-release/semantic-release) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also want to include prettier
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!
|
||
> NOTE: This library is still in **beta** as we flesh out the API of the SDK. | ||
|
||
## ToC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to look into generating the ToC, may be out of scope for this just a consideration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe...
Not sure how I missed the Also, I wish I knew about npm 6 years ago. That was life changing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
@@ -1,13 +1,20 @@ | |||
language: node_js | |||
node_js: | |||
- "node" | |||
- "10" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why add 10 if node is already at 10?
The intention of this PR was just to write the docs for the `request()` method. But while writing the docs, I realized that the error handling was broken in certain cases. So that got fixed along with the tests. So what ended up happening: - Wrote docs for configuring an SDK object - Wrote docs for lowest-level `request()` method - Made `request()` the only export in `request.tsc` - Fixed error handling in `request.tsc` - Updated tests to only test `request()` and to test for more error cases - Moved `PULL_REQUEST_TEMPLATE.md` & `CONTRIBUTING.md` from #33
When running the build on Node 6, we only have NPM 3, which doesn't have `npx`. So by adding the deploy `on` filter to run `semantic-release` on the latest node version, we won't try to even run it on Node 6. BREAKING CHANGE: We need to trigger a version bump after taking over the package. Because #33 was just a `chore()`, `semantic-release` assumed there was nothing to release.
🎉 This PR is included in version 1.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
Updates
.travis.yml
to have a deploy step that callssemantic-release
withnpx
.Also updated the
README
with a bunch of new content in anticipation of the library being released.Lastly did some misc updates to
CONTRIBUTING.md
&PULL_REQUEST_TEMPLATE.md
Fixes #17
How Has This Been Tested?
There's no real way to test this besides actually releasing.
Screenshots (if appropriate):
n/a
Checklist:
yarn validate
to ensure that tests, typescript and linting are all in order.