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

Add travis CI integration #7

Merged
merged 2 commits into from
Jul 21, 2017
Merged

Conversation

LucianBuzzo
Copy link
Contributor

Connects to #2

change-type: patch

@LucianBuzzo LucianBuzzo self-assigned this Jul 21, 2017
package.json Outdated
"main": "src/index.js",
"typings": "src/index.d.ts",
"scripts": {
"test": "mocha -r ts-node/register --outDir tmp test/index.spec.ts",
"build": "tsc src/index.ts"
"build": "tsc src/index.ts",
"ci": "npm run test"

Choose a reason for hiding this comment

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

Don't think you need this - if you drop this script, and don't specify a script in .travis.yml at all then it'll run npm test by default anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm adding a lint script in a follow up pr, so the ci script will become npm run lint && npm test.

Choose a reason for hiding this comment

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

Normal developers should run that too though, and normal developers running npm run ci feels weird.

Generally, I'd make npm test the go-to entry point to test everything - it's nice and intuitive and it's what everybody & everything will run by default - and make test run npm run lint && npm run mocha.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok cool 👍 I'll drop the ci script

package.json Outdated
"chai": "4.1.0",
"mocha": "3.4.2",
"ts-node": "3.2.1",
"typescript": "2.4.2"

Choose a reason for hiding this comment

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

I don't think we should do this, because there's some downsides, it's inconsistent with most/all our other modules, and there's a better option.

Practically, this is going to mean npm can't dedupe dependencies (i.e. if this depends on semver 5.3.0 and another module uses semver 5.3.1, both will have to be installed and webpack bundled). That's bad for bundle and install sizes, but if you have slightly different behaviour between the two modules it can lead to all sorts of crazy fun bugs where things only break in half your application.

More fluffily, while breaking patch releases are super annoying, it's generally better to enforce any version fixing we need at the top level (i.e. the UI). There we can use something that works recursively, which is more reliable and easier to manage. If we want to fix versions just for development on this module itself here, but not for downstream consumers, we should use npm5 or yarn, which can happily do that.

Choose a reason for hiding this comment

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

(typescript is one exception I think you can probably get away with here, since it's pretty major and I know we've had specific problems there, but for other packages I think this holds)

package.json Outdated
@@ -1,27 +1,28 @@
{
"name": "resin-semver",
"version": "0.1.1",
"description": "resin specific semver utility methods",
"description": "Resin.io specific semver utility methods",
"main": "src/index.js",
"typings": "src/index.d.ts",
"scripts": {
"test": "mocha -r ts-node/register --outDir tmp test/index.spec.ts",

Choose a reason for hiding this comment

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

Why do we need the ourDir here? I think most of our other modules just don't bother.

Connects to #2

change-type: patch
@LucianBuzzo LucianBuzzo changed the title Add travis CI integration and lock package versions Add travis CI integration Jul 21, 2017
@LucianBuzzo LucianBuzzo dismissed pimterry’s stale review July 21, 2017 14:26

Issues are addressed

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

3 participants