Summary

Always use pipeline compilers, add babel.

Review Request #7954 — Created Feb. 10, 2016 and submitted Feb. 23, 2016, 8:49 a.m.

Information

Owner

david

Repository

Review Board

Branch

release-2.6.x

Bugs

Depends On

Commit

46926cc...

Reviewers

Groups

reviewboard

People

Description

This change switches us over to using pipeline's compilers all the time, not
just when PIPELINE_ENABLED = True, as well as adding the babel compiler to
the list. This means that compilation of LESS (and ES6!) happens on the
back-end instead of the front-end during development.

This is better than before because instead of recompiling all the stylesheets
on page load, we do it on-demand, and pipeline does mtime checks to verify
whether it needs to actually do any work. This significantly speeds up reloads
during development.

This adds new dev dependencies on lessc and babel. These are included in a
package.json file that allows one to just run "npm install" to get everything
necessary.

Testing Done


Verified that recompile on-demand from within the devserver worked as

  expected.
Converted common.js to common.es6 and made use of some ES6 features. Verified

  that what I got in the browser was transpiled ES5.
Converted a JS file in an extension to ES6 and saw similar results.

Issues

Description	From	Last Updated
'django_reset' imported but unused	reviewbot	Feb. 10, 2016, 12:47 p.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	Feb. 10, 2016, 12:47 p.m.
We don't use @param do we?	brennie	Feb. 10, 2016, 2:24 p.m.
Missing semicolon.	ME medanat	Feb. 10, 2016, 1:35 p.m.
`${$target.attr('href')}infobx/`	ME medanat	Feb. 10, 2016, 1:35 p.m.
infobox	brennie	Feb. 10, 2016, 1:35 p.m.
We use these flags for more than media building. They should stay.	chipx86	Feb. 10, 2016, 1:25 p.m.
'django_reset' imported but unused	reviewbot	Feb. 10, 2016, 2:45 p.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	Feb. 10, 2016, 2:45 p.m.
'djblets' imported but unused	reviewbot	Feb. 17, 2016, 10:16 a.m.
'django_reset' imported but unused	reviewbot	Feb. 17, 2016, 10:16 a.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	Feb. 17, 2016, 10:16 a.m.
'django_reset' imported but unused	reviewbot	Feb. 17, 2016, 10:19 a.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	Feb. 17, 2016, 10:19 a.m.

Tool: Pyflakes
Processed Files:
    reviewboard/settings.py
    reviewboard/test.py
    tests/runtests.py
    reviewboard/staticbundles.py

Ignored Files:
    package.json
    reviewboard/static/rb/js/common.es6
    reviewboard/templates/base.html
    reviewboard/templates/js/pipeline.html
    .gitignore
    reviewboard/static/rb/js/.jshintrc



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/settings.py
    reviewboard/test.py
    tests/runtests.py
    reviewboard/staticbundles.py

Ignored Files:
    package.json
    reviewboard/static/rb/js/common.es6
    reviewboard/templates/base.html
    reviewboard/templates/js/pipeline.html
    .gitignore
    reviewboard/static/rb/js/.jshintrc

reviewboard/settings.py (Diff revision 1)
The issue has been dropped. Show all issues
```
 'django_reset' imported but unused
```
reviewboard/settings.py (Diff revision 1)
The issue has been dropped. Show all issues
```
 'from settings_local import *' used; unable to detect undefined names
```

reviewboard/static/rb/js/common.es6 (Diff revision 1)

Is there a reason the semicolon was removed here?

reviewboard/static/rb/js/common.es6 (Diff revision 1)
```
This can be a const I believe.
```
reviewboard/static/rb/js/common.es6 (Diff revision 1)
```
`#${id}` instead of '#' + id maybe?
```

```
I will be so happy to write ES6 :)
```

reviewboard/static/rb/js/common.es6 (Diff revision 1)

The issue has been dropped. Show all issues

We don't use @param do we?

david Feb. 10, 2016, 1:37 p.m.

No but I don't want to make this too invasive right now. common.js is pretty old and crufty stuff anyway and should eventually go away--I just wanted a proof of concept.

reviewboard/static/rb/js/common.es6 (Diff revision 1)
The issue has been resolved. Show all issues
```
Missing semicolon.
```
reviewboard/static/rb/js/common.es6 (Diff revision 1)
The issue has been resolved. Show all issues
```
`${$target.attr('href')}infobx/`
```

reviewboard/static/rb/js/common.es6 (Diff revision 1)
The issue has been resolved. Show all issues
```
infobox
```

Awesome. It'll be really nice to be able to have the smarter recompilation and support for ES6 :)

We're discussing it in Slack already, but for posterity, the things I'm hoping we can change are:

1) Using .js (or .es6.js) for the files instead, so everyone in the world (including Pygments) doesn't have to support .es6. Also, having a version in there makes it hard down the road for ES7, etc.
2) I'd love to put built files in a separate directory, so we don't have a bunch of stuff all coming up when you grep. Something more manageable would be nice. Also, if we could do this, no need for .es6 at all.

david Feb. 10, 2016, 1:26 p.m.

Well, the built files do end up going into reviewboard/htdocs/static (nothing writes to reviewboard/static anymore), but the problem is that pipeline uses a two-stage system where first they do a "collect" (which copies all the files into htdocs/static/), and then "compile" (which compiles things). We therefore can't have the same filename for input and output.
I can implement a custom compiler subclass that does whatever path we want. I figure .es6.js is fine since I suspect the tooling will change before the next version of ES is codified.

reviewboard/test.py (Diff revision 1)

The issue has been dropped. Show all issues

We use these flags for more than media building. They should stay.

david Feb. 10, 2016, 1:26 p.m.

These are now set earlier. runtests.py sets the environment variable, and settings.py sets the setting.

Change Summary:


Use new compiler in djblets that does filenames the way we want.
Move common.es6 to common.es6.js.
Turn on source maps for babel.
Fix a few issues in common.es6.js that reviewers pointed out.

Commit:

7279d5deb8e5bbd4d642dac5da0b9635537eb008

ac31b7ad34ef0fd5b4d42d65d287d13273c9ac0d

Diff:

Revision 2 (+74 -97)

Show changes

	.gitignore
	package.json
	reviewboard/settings.py
	reviewboard/staticbundles.py
	reviewboard/test.py
	reviewboard/static/rb/js/.jshintrc
	reviewboard/static/rb/js/common.js
	reviewboard/templates/base.html
	2 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/settings.py
    reviewboard/test.py
    tests/runtests.py
    reviewboard/staticbundles.py

Ignored Files:
    package.json
    reviewboard/templates/base.html
    reviewboard/templates/js/pipeline.html
    .gitignore
    reviewboard/static/rb/js/common.es6.js
    reviewboard/static/rb/js/.jshintrc



Tool: Pyflakes
Processed Files:
    reviewboard/settings.py
    reviewboard/test.py
    tests/runtests.py
    reviewboard/staticbundles.py

Ignored Files:
    package.json
    reviewboard/templates/base.html
    reviewboard/templates/js/pipeline.html
    .gitignore
    reviewboard/static/rb/js/common.es6.js
    reviewboard/static/rb/js/.jshintrc

reviewboard/settings.py (Diff revision 2)
The issue has been dropped. Show all issues
```
 'django_reset' imported but unused
```
reviewboard/settings.py (Diff revision 2)
The issue has been dropped. Show all issues
```
 'from settings_local import *' used; unable to detect undefined names
```

Ship it!

Your testing done needs to be updated re: .es6.js vs .es6.

Ship it!

```
Ship It!
```

Change Summary:

Simplify less arguments.

Commit:

ac31b7ad34ef0fd5b4d42d65d287d13273c9ac0d

3f154b7145440ad0bd41d924882562d252e430a7

Diff:

Revision 3 (+84 -119)

Show changes

	.gitignore
	package.json
	contrib/internal/conf/settings_local.py
	reviewboard/settings.py
	reviewboard/staticbundles.py
	reviewboard/test.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/.jshintrc
	4 more

Tool: Pyflakes
Processed Files:
    tests/runtests.py
    reviewboard/settings.py
    reviewboard/test.py
    contrib/internal/conf/settings_local.py
    reviewboard/staticbundles.py

Ignored Files:
    package.json
    reviewboard/templates/base.html
    reviewboard/templates/js/pipeline.html
    .gitignore
    reviewboard/static/rb/js/common.es6.js
    reviewboard/static/rb/js/.jshintrc
    reviewboard/static/rb/css/pages/reviews.less



Tool: PEP8 Style Checker
Processed Files:
    tests/runtests.py
    reviewboard/settings.py
    reviewboard/test.py
    contrib/internal/conf/settings_local.py
    reviewboard/staticbundles.py

Ignored Files:
    package.json
    reviewboard/templates/base.html
    reviewboard/templates/js/pipeline.html
    .gitignore
    reviewboard/static/rb/js/common.es6.js
    reviewboard/static/rb/js/.jshintrc
    reviewboard/static/rb/css/pages/reviews.less

contrib/internal/conf/settings_local.py (Diff revision 3)
The issue has been resolved. Show all issues
```
 'djblets' imported but unused
```
reviewboard/settings.py (Diff revision 3)
The issue has been dropped. Show all issues
```
 'django_reset' imported but unused
```
reviewboard/settings.py (Diff revision 3)
The issue has been dropped. Show all issues
```
 'from settings_local import *' used; unable to detect undefined names
```

Commit:

3f154b7145440ad0bd41d924882562d252e430a7

46926cc12e00df89b58810e2dcdcdfc2e248a1bb

Diff:

Revision 4 (+84 -121)

Show changes

	.gitignore
	package.json
	contrib/internal/conf/settings_local.py
	reviewboard/settings.py
	reviewboard/staticbundles.py
	reviewboard/test.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/.jshintrc
	4 more

Tool: PEP8 Style Checker
Processed Files:
    tests/runtests.py
    reviewboard/settings.py
    reviewboard/test.py
    contrib/internal/conf/settings_local.py
    reviewboard/staticbundles.py

Ignored Files:
    package.json
    reviewboard/templates/base.html
    reviewboard/templates/js/pipeline.html
    .gitignore
    reviewboard/static/rb/js/common.es6.js
    reviewboard/static/rb/js/.jshintrc
    reviewboard/static/rb/css/pages/reviews.less



Tool: Pyflakes
Processed Files:
    tests/runtests.py
    reviewboard/settings.py
    reviewboard/test.py
    contrib/internal/conf/settings_local.py
    reviewboard/staticbundles.py

Ignored Files:
    package.json
    reviewboard/templates/base.html
    reviewboard/templates/js/pipeline.html
    .gitignore
    reviewboard/static/rb/js/common.es6.js
    reviewboard/static/rb/js/.jshintrc
    reviewboard/static/rb/css/pages/reviews.less

reviewboard/settings.py (Diff revision 4)
The issue has been dropped. Show all issues
```
 'django_reset' imported but unused
```
reviewboard/settings.py (Diff revision 4)
The issue has been dropped. Show all issues
```
 'from settings_local import *' used; unable to detect undefined names
```

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-2.6.x (cfefb40)