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

Improve build tool to make it easier to maintain and more pleasant to use #21777

Closed
wants to merge 26 commits into from

Conversation

pvdlg
Copy link
Contributor

@pvdlg pvdlg commented Jan 19, 2017

Reopened here #21929

Following #20332 I've spent some time trying to improve the build process by switching to npm scripts only. See comment.
My analysis is that npm scripts, while looking incredibly simple in theory, present several constraints and that the problems of the build tool are not necessarly related to the tools but mostly the way tasks are organized and orchestrated.

So I decided to evaluate a solution that would make the build tool simpler to use, easier to maintain, robust and more importantly pleasant to use for developers, while building as much as possible on the existing that works.

It turns out it was not that hard and really fun to do :-), so after a couple hours the evaluation turned something that was almost there. So I decided to polish it, update the doc and propose a PR so anyone can can have a look.
Image speak more than words so this is what I have now:

bootstrap-new-build

Here is the improvement that this PR brings.

Easy setup

npm install and that's it.

Simple to use

Only 3 commands for the user/developer:

  • dist: build JS, CSS and Docs.
  • test: build JS, CSS, Docs, test and lint.
  • watch: Start everything to dev efficiently.

Only 4 commands for the maintainer:

  • prep-release: prepare release.
  • publish: publish doc on GitHub.
  • change-version: update version in source files.
  • update-shrinkwrap: update shrinkwrap dependencies management.

Pleasant to develop with

npm run watch, Open your browser and voila! All changes made in scss, js and docs are automatically reflected in live on the local doc website. See gif above :)

Fast

  • build runs in 6s (it used to be 24s)
  • test runs in 20s
  • watch react to a modification of js, scss or docs files, handle everything
    and reload the docs on the browser in about 1s (it used to be up to 10s just for sass + time for Jekyll to process the change)

Easier to maintain

  • Gruntfile is now 10 line of code and shouldn't have to be modified anymore.
  • For each task there is a short declarative config file under build/grunt.
  • Grunt tasks have been reorganized in a more logical way and can be found in build/grunt/aliases.js.
  • No more custom JS script.
  • No more Javascript function here and there (like in current Gruntfile.js).
  • Most of the Javascript code has been eliminated. Only the meaningful configuration remains in build/grunt and build/grunt/aliases.js and is written in a more declarative way than it would in an npm script.

Stable

  • Most of the individual tasks are the one Bootstrap has been running for a while now, so that limit the risk of regression.
  • All the Grunt plugins are reliable and well maintained plugins (often more up to date than some npm cli equivalent).

How do I use it?

  • git clone -b build-rework https://github.com/vanduynslagerp/bootstrap
  • cd bootstrap
  • npm install
  • npm run watch
  • Open you browser to localhost:9001
  • Start coding! Each modification is applied within 1s and the doc website is reloaded automatically. Just code and watch the result live.

I understand the Bootstrap team, like many others, seems to desire to move to pure npm scripts. That said, I think I reached a pretty satisfying result, that worth to be evaluated, which is the point of this PR.

options: {
test() {
return typeof process.env.SAUCE_ACCESS_KEY !== 'undefined' && (!process.env.TWBS_TEST || process.env.TWBS_TEST ===
'sauce-js') && (process.env.TWBS_DO_SAUCE === undefined || process.env.TWBS_DO_SAUCE !== '0')

Choose a reason for hiding this comment

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

Unexpected use of process.env no-process-env
'process' is not defined no-undef

'sauce-js': {
options: {
test() {
return typeof process.env.SAUCE_ACCESS_KEY !== 'undefined' && (!process.env.TWBS_TEST || process.env.TWBS_TEST ===

Choose a reason for hiding this comment

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

Unexpected use of process.env no-process-env
'process' is not defined no-undef

options: {
test() {
return (!process.env.TWBS_TEST || process.env.TWBS_TEST === 'validate-html') && (process.env.TWBS_DO_VALIDATOR ===
undefined || process.env.TWBS_DO_VALIDATOR !== '0')

Choose a reason for hiding this comment

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

Unexpected use of process.env no-process-env
'process' is not defined no-undef

'validate-html': {
options: {
test() {
return (!process.env.TWBS_TEST || process.env.TWBS_TEST === 'validate-html') && (process.env.TWBS_DO_VALIDATOR ===

Choose a reason for hiding this comment

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

'process' is not defined no-undef
Unexpected use of process.env no-process-env

core: {
options: {
test() {
return (!process.env.TWBS_TEST || process.env.TWBS_TEST === 'core') && process.env.TRAVIS_REPO_SLUG !==

Choose a reason for hiding this comment

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

Unexpected use of process.env no-process-env
'process' is not defined no-undef

@@ -0,0 +1,29 @@
module.exports = {

Choose a reason for hiding this comment

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

'module' is not defined no-undef

@@ -0,0 +1,8 @@
module.exports = {

Choose a reason for hiding this comment

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

'module' is not defined no-undef

@@ -0,0 +1,28 @@
module.exports = {

Choose a reason for hiding this comment

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

'module' is not defined no-undef

@@ -0,0 +1,14 @@
module.exports = {

Choose a reason for hiding this comment

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

'module' is not defined no-undef

@@ -0,0 +1,20 @@
module.exports = {

Choose a reason for hiding this comment

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

'module' is not defined no-undef

@@ -0,0 +1,27 @@
module.exports = {

Choose a reason for hiding this comment

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

'module' is not defined no-undef

@@ -0,0 +1,18 @@
module.exports = {

Choose a reason for hiding this comment

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

'module' is not defined no-undef

@@ -0,0 +1,24 @@
module.exports = {

Choose a reason for hiding this comment

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

'module' is not defined no-undef

@@ -0,0 +1,45 @@
module.exports = {

Choose a reason for hiding this comment

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

'module' is not defined no-undef

@@ -0,0 +1,24 @@
module.exports = {

Choose a reason for hiding this comment

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

'module' is not defined no-undef

@@ -0,0 +1,26 @@
module.exports = {

Choose a reason for hiding this comment

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

'module' is not defined no-undef

@pvdlg pvdlg mentioned this pull request Jan 19, 2017
18 tasks
},
// Skip Sauce on Travis when [skip sauce] is in the commit message
isDoSauceJs() {
return this.isSauceJs() && isUndefOrNonZero(process.env.TWBS_DO_SAUCE)

Choose a reason for hiding this comment

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

Unexpected use of process.env no-process-env
'process' is not defined no-undef

// Only run Sauce Labs tests if there's a Sauce access key
// and skip Sauce if running a different subset of the test suite
isSauceJs() {
return typeof process.env.SAUCE_ACCESS_KEY !== 'undefined' && runSubset('sauce-js-unit')

Choose a reason for hiding this comment

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

Unexpected use of process.env no-process-env
'process' is not defined no-undef

// Skip HTML validation if running a different subset of the test suite
// or [skip validator] is in the commit message
isValidateHtml() {
return runSubset('validate-html') && isUndefOrNonZero(process.env.TWBS_DO_VALIDATOR)

Choose a reason for hiding this comment

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

Unexpected use of process.env no-process-env
'process' is not defined no-undef

// Skip core tests if running a different subset
// of the test suite or if this is a Savage build
isCore() {
return runSubset('core') && process.env.TRAVIS_REPO_SLUG !== 'twbs-savage/bootstrap'

Choose a reason for hiding this comment

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

Unexpected use of process.env no-process-env
'process' is not defined no-undef

module.exports = function () {

function runSubset(subset) {
return !process.env.TWBS_TEST || process.env.TWBS_TEST === subset

Choose a reason for hiding this comment

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

Unexpected use of process.env no-process-env
'process' is not defined no-undef

@@ -0,0 +1,32 @@
module.exports = function () {

Choose a reason for hiding this comment

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

'module' is not defined no-undef

branch: 'gh-pages'
}
require('time-grunt')(grunt)
require('load-grunt-config')(grunt, {

Choose a reason for hiding this comment

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

Unexpected require() global-require
'require' is not defined no-undef

remote: '[email protected]:twbs/derpstrap.git',
branch: 'gh-pages'
}
require('time-grunt')(grunt)

Choose a reason for hiding this comment

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

Unexpected require() global-require
'require' is not defined no-undef

@bardiharborow
Copy link
Member

This is interesting, but a heads-up that I can't guarantee we'll use this.

@pvdlg
Copy link
Contributor Author

pvdlg commented Jan 22, 2017

@bardiharborow that's alright. I continue to improve it whenever I see something because I use it when I work on Bootstrap. The gain in comfort and productivity is really appreciable.
So even if Bootstrap decide to not go that way, it's not lost as I would have used it for a while.

Btw, let me know if you want me to improve/change something that would help this build tool to be adopted.

@twbs-rorschach
Copy link

Hi @vanduynslagerp!

Thanks for wanting to contribute to Bootstrap by sending this pull request!
Unfortunately, your pull request seems to have some problems:

You'll need to fix these mistakes and revise your pull request before we can proceed further.
Once you've fixed these problems, you can either ask the maintainers to re-open this pull request, or you can create a new pull request.
Thanks!

(Please note that this is a fully automated comment.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants