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

chore(build): Add semantic-release in Travis #33

Merged
merged 7 commits into from
Jun 22, 2018

Conversation

benmvp
Copy link
Contributor

@benmvp benmvp commented May 3, 2018

Description

Updates .travis.yml to have a deploy step that calls semantic-release with npx.

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

NOTE: This cannot be merged until we take ownership of the eventbrite NPM package

How Has This Been Tested?

There's no real way to test this besides actually releasing.

Screenshots (if appropriate):

n/a

Checklist:

  • I have read the CONTRIBUTING document.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • I have run yarn validate to ensure that tests, typescript and linting are all in order.

Copy link
Contributor

@kwelch kwelch left a 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"
Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

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.

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?

Copy link
Contributor Author

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

Copy link
Contributor

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?

Copy link
Contributor Author

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
Copy link
Contributor

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?

Copy link
Contributor Author

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...

@benmvp
Copy link
Contributor Author

benmvp commented May 3, 2018

Can't go live until we get control of the eventbrite package

@kwelch
Copy link
Contributor

kwelch commented May 3, 2018

Wow, that was published 6 years ago. 😳

@benmvp
Copy link
Contributor Author

benmvp commented May 3, 2018

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)
Copy link
Contributor

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?

Copy link
Contributor Author

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
Copy link
Contributor

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.

https://github.com/ekalinin/github-markdown-toc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe...

@kwelch
Copy link
Contributor

kwelch commented May 4, 2018

Not sure how I missed the ReadMe.md updates earlier, but they look great.

Also, I wish I knew about npm 6 years ago. That was life changing.

Copy link

@rwholey-eb rwholey-eb left a 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"

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?

@BenAtEventbrite BenAtEventbrite added this to the Alpha milestone May 17, 2018
benmvp referenced this pull request in benmvp/eventbrite-sdk-javascript May 17, 2018
BenAtEventbrite pushed a commit that referenced this pull request May 17, 2018
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
@benmvp benmvp force-pushed the semantic-release branch from 1e3f523 to 764bdc9 Compare May 17, 2018 00:24
@BenAtEventbrite BenAtEventbrite merged commit 0161a67 into eventbrite:master Jun 22, 2018
BenAtEventbrite pushed a commit that referenced this pull request Jun 22, 2018
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.
@ebtravis
Copy link
Collaborator

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

6 participants