-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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: move to circleci #2390
chore: move to circleci #2390
Conversation
package.json
Outdated
} | ||
"name": "rollup", | ||
"version": "0.64.1", | ||
"description": "Next-generation ES module bundler", |
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.
Could you not do this prettier change?
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.
WIP :) this is not yet ready for review. Please hold off until that tag is removed.
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.
@TrySound it's ready 🎉
This PR is ready for review. A few notes:
The following tasks need to be performed after this PR has been approved, but before merging:
|
I'm just wondering what would we need to do to get the CircleCI check to display in the PR here? |
@guybedford see my previous comment. it's a global setting in CircleCI. If I turn it on here, it tends to display false positives on other PRs. To view the last build of this branch: https://circleci.com/gh/rollup/rollup/tree/chore%2Ftravis-to-circle |
The times do look pretty good there I must say. Perhaps this will even help with our watcher file descriptor flake issues :) |
@@ -10,9 +10,9 @@ init: | |||
environment: | |||
matrix: | |||
# node.js | |||
- nodejs_version: 4 |
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 should keep Node 4 for now I think.
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.
I think you may remove node4 support with 0.65 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.
Node 4 has been out of active and maintenance LTS since April, and has passed end of live and is unsupported. (https://github.com/nodejs/Release#end-of-life-releases) This is a prudent change.
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.
Fair enough. Given that I agree with @shellscape to remove it for 0.65
. But we should have a discussion before we actually change the TypeScript output target to match that in the future as this would have some hidden implications.
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.
Debian stable packages 4.8.2, and since sindresorhus/pretty-ms@b5a583d, rollup doesn't run on nodejs 4 anymore.
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.
This has my approval, but @lukastaegert please check this one as well before merge.
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.
Great work. One final note from me: Currently, npm run lint
uses the --fix
options. While this is probably exactly what we want locally and in the commit hook, the check on CI should probably skip the fixing since it is not committing any changes after that.
Instead I would prefer if CI would run a different script, e.g. npm run lint:ci
, that does not auto-correct so that we are actually sure only valid code is merged.
@lukastaegert requested changes made! |
package.json
Outdated
"ci:test": "npm run test && npm run build:bootstrap && npm run test:only", | ||
"lint": "tslint --project . --fix && eslint --fix test/test.js test/*/index.js test/utils.js test/**/_config.js", | ||
"lint:nofix": "tslint --project . && eslint test/test.js test/*/index.js test/utils.js test/**/_config.js", |
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.
👍
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.
Nice! From my side you can merge and switch CIs at any time
@lukastaegert thanks! The last build for the merge succeeded (https://circleci.com/gh/rollup/rollup/99), so we're good to go. |
This PR contains:
Breaking Changes?
If yes, please describe the breakage.
Please Describe Your Changes
This PR proposes to move from Travis CI to Circle CI. Benefits will include: