Webpack

[sebastienbarre]

Here we go.

Scope

This PR holds all changes related to the ES6/ES2015 modularization of the Girder web client and 20+ of its plugins, as well as the subsequent bundling using Webpack.

Almost all source files have been updated to no longer rely on the global girder object/singleton. Specific functionalities, Jade templates, as well as Stylus/CSS styles must now be imported explicitly. For example:

import _ from 'underscore';

import FileModel from 'girder/models/FileModel';
import View from 'girder/views/View';
import events from 'girder/events';
import { renderMarkdown, formatSize } from 'girder/misc';

import MarkdownWidgetTemplate from 'girder/templates/widgets/markdownWidget.jade';
import 'girder/stylesheets/widgets/markdownWidget.styl';

var MarkdownWidget = View.extend({
    events: {
        'show.bs.tab .g-preview-link': function () {
            renderMarkdown(this.val().trim() || 'Nothing to show', this.$('.g-markdown-preview'));
        },
...
});

export default MarkdownWidget;

Importing on a "per-needed" basis will provide the added benefit of facilitating tree shaking and build-time checks.

Directory Structure

The directory structure in clients\web\src was not altered, but some functions/classes were moved to their own files. For example:

• authentication-related code is now in auth.js,
• constants are now in constants.js,
• dialog (UI) related code is in dialog.js,
• the event emitter is now in events.js,
• miscellaneous functions are now in misc.js,
• REST-related code is now in rest.js,
• the router is now in router.js,
• routes definitions are all in routes.js,
• server related code is in server.js,
• collection.js was moved to collections\Collection.js,
• model.js was split into Model.js, AccessControlledModel.js and MetadataMixin.js inside the models subdir,
• plugin_utils.js is now in utilities\PluginUtils.js,
• jQuery plugins are now in the utilities\jquery subdir,

Plugins

All plugins were cleaned-up to adopt the same directory structure and naming conventions as the core files, and to better convey which core files are being extended or wrapped. Specifically:

• all plugins feature a main.js entry point,
• routes, if any, are defined in routes.js,
• views are to be found in a views subdir,
• all templates, stylesheets, and views were renamed, when needed, to be consistent with each-others (e.g. views/ItemPreviewWidget.js, templates/itemPreviewWidget.jade, stylesheets/itemPreviewWidget.styl),
• any core model/collection/view being extended prototypically or wrap()'ed is now to be found in a single file matching the same model/collection/view name and location (say, plugins/curation/web_client/views/HierarchyWidget.js).

Migration

I would recommend checking any collection, model, view, or plugin to help transitioning your own code. Search for instances of the girder. string to find the functions previously attached to the girder global namespace, and bring them into your code using import... from.... Export your own code using export default ... to make it available to the outside world as a module. Start a new plugin by copy/pasting an existing one.

Note that it is still possible, although not recommended, to use a higher-level namespace.
For example, the old:

girder.fetchCurrentUser(blah blah);

is now coded this way:

import { fetchCurrentUser } from 'girder/auth';
[...]
fetchCurrentUser(blah blah);

yet, it could also be written this way:

import * as auth from 'girder/auth';
[...]
auth.fetchCurrentUser(blah blah);

...but your bundler of choice (say, Webpack) would no longer be able to check at build-time if fetchCurrentUser is to be found in girder/auth. It could also be rewritten as:

import * as girder from 'girder';
[...]
girder.auth.fetchCurrentUser(blah blah);

... but the risk here would be even higher, should the auth namespace or its contents be moved or renamed.

Be aware that if you passed the girder global to a Jade template, you will now need to pass individual functions separately, and update the Jade template accordingly. For example:

in clients/web/src/views/body/CollectionsView.js:

       this.$el.html(girder.templates.collectionList({
             collections: this.collection.toArray(),             
             girder: girder
         }));

becomes:

import { formatSize } from 'girder/misc';
import { getCurrentUser } from 'girder/auth';
import CollectionListTemplate from 'girder/templates/body/collectionList.jade'; 
[...]
       this.$el.html(CollectionListTemplate({
             collections: this.collection.toArray(),
             getCurrentUser: getCurrentUser,
             formatSize: formatSize
         }));

and in the corresponding clients/web/src/templates/body/collectionList.jade

if girder.currentUser
[...]
        span.g-user-space-used  #{girder.formatSize(collection.get('size'))}

becomes:

if getCurrentUser()
[...]
        span.g-user-space-used  #{formatSize(collection.get('size'))}

Also note that a girder global is still available to facilitate testing, but only after the core Javascript bundles and all plugins have been loaded. It is nearly identical to the previously available global, but some functions were shuffled around, as described in the "Directory Structure" section.

Tip: use console.dir(girder) to browse the new tree.

$ console.dir(girder)
> views
> auth
> collections
> utilities
> server
> dialog
  > confirm
     [...]
[...]

then follow the same "path" to migrate to the new import...from statement:

import { confirm } from 'girder/dialog';

Importing from one plugin to another can be done using the plugins/ prefix. For example,
consider JobStatus being exported by the jobs plugin in /plugins/jobs/web_client/JobStatus.js:

var JobStatus = {
    registerStatus: function (status) {
        [...]
    }
}
export default JobStatus;

then later on used by the worker plugin in /plugins/worker/web_client/JobStatus.js, like this (note that there is no web_client subdir here):

import JobStatus from 'plugins/jobs/JobStatus';
JobStatus.registerStatus({
    WORKER_FETCHING_INPUT: {
        [...]
    }
});

TODO

• This work was done on Mac OSX, which is case-insensitive -- there may be some lingering, trivial file naming issues that Unix users will notice right away.

Grunt Build-process

The old, Grunt build process is still in place. I do not advocate removing it actually, as Webpack is a bundler, not a task runner. It was nonetheless slimmed down, as grunt_tasks/build.js is almost 100% Webpack code at this point. A few Grunt plugins remain, and could potentially be removed in favor of Webpack (see such plugins as kevlened/copy-webpack-plugin as a replacement to grunt-contrib-copy).

• Testing: grunt_tasks/dev.jsstill uses grunt-contrib-uglify; it needs to be exercised, converted to the new modules, and likely fixed before it is Webpack'ed; UPDATE: soon to be webpack'ed,
• Packaging: grunt_tasks/package.js uses grunt-shell; it needs to be exercised and likely fixed,
• Fontello: grunt_tasks/fontello.js uses grunt-contrib-copy and grunt-fontello; it is functional and would likely be fine as is,
• Plugins: grunt_tasks/plugin.js still uses grunt-contrib-copy and grunt-npm-install; the code that handles Javascript, Jade, and Stylus for each plugin was moved and integrated into the single Webpack target in grunt_tasks/build.js; while plugins appear to be generated correctly, **a large section (starting at grunt.file.expand()) needs to be exercised and likely be fixed or removed*,
• Sphinx: grunt_tasks/sphinx.js uses grunt-shell; it appears functional and would likely be fine left as is,
• Swagger: grunt_tasks/swagger.js uses grunt-contrib-copy and grunt-contrib-stylus; it is functional but could be Webpack'ed, or left as is,
• Versioning: grunt_tasks/version.js uses grunt-file-creator and grunt-gitinfo; it is functional and would likely be fine as is,

Webpack

The Webpack target generates, in clients\web\static\built:

• girder.ext.min.js and girder.ext.min.css (and corresponding source maps): all third-party/vendor Javascript and CSS (e.g. imported from node_modules),
• girder.app.min.js and girder.app.min.css (and corresponding source maps): all Girder core Javascript/Jade/CSS (e.g. found in clients\web\src) used by the client and the plugins,
• plugin.min.js and plugin.min.css (and corresponding source maps) for each plugin, in plugins\name_of_plugin: plugin code only, typically small, making calls into girder.app (and girder.ext),
• glyphicons in the assets subdir

However:

• There still appears to be too much duplicated code going in girder.main.min.js and each plugin.min.js; this needs to be investigated and fixed by myself (Seb; consider robertknight/webpack-bundle-size-analyzer), also consider the DllPlugin, which would increase performance as well (Webpack Plugins we been keepin on the DLL, Optimizing Webpack for Faster React Builds, Optimizing Webpack build times and improving caching with DLL bundles, webpack/examples/dll). Also check trivago/parallel-webpack: Builds multi-config webpack projects in parallel
• The Grunt watch target in build.js needs to be converted to a similar capability compatible with Webpack; (use grunt --watch or grunt webpack:watch, for grunt-contrib-watch is too slow),
• Support for ES2015 code through Babel should be enabled (by myself, Seb),
• Importing $.girderModal() looks too much like a side effect: import 'girder/utilities/jQuery';; let's find a way to make it more explicit (check how jQuery does it nowadays), or restructure the files so that we have one prototype method per file, so that it would be more obvious why it's getting imported, e.g. import 'girder/utilities/jQuery/girderModal';
• It is still unclear at this point how minimization and debugging will ultimately be enabled in Webpack 2.0 (see What's new in webpack 2, Why does the UglifyJsPlugin affect other loaders?, this comment), so the 2 (or 3) different ways to do so are used at the same time (see how environment and debugJs are used in grunt_tasks/build.js), but this will need some clean up later on,

Testing

• Programmatic testing of the web client was disabled, and needs to be converted and re-enabled ASAP, or Continuous Integration will fail (see grunt_tasks/dev.js, remove early return in (grunt.registerTask); I (Seb) could use some help for this one, from the original testing author Update: most tests are passing, but the way the test driver exists make it look like they are failing on some laptops,

Plugins

All plugins were converted. Manual testing was performed to check that each plugin's UI is in place, using eye-ball technology.

• Programmatic testing, if any, needs to be converted and re-enabled ASAP (say, plugins\autojoin\plugin_tests\autoJoin.spec)

Misc, Minor

• Consider moving constants from constants.js back to more relevant files, closer to the functions that most often use them?
• Consider files like auth.js, events.js, rest.js, router.js, which essentially bundle utility functions, vs. the files in utilities such as DialogHelper.js, MiscFunctions.js, or PluginUtils.js, which also bundle utility functions. Are the former considered more important "utilities" then the latter? Or should they all be in the utilities dir? Should some be merged somehow in the process? Or named more consistently?
• girder/router exports { router, splitRoute }. Therefore, accessing the router from the girder singleton will look like girder.router.router; not ideal; consider renaming girder/router to girder/routing to use girder.routing.router instead?
• girder/events exports { events, eventSream }. Therefore, accessing the events object from the girder singleton will look like girder.events.events; not ideal; consider renaming girder/events to girder/eventing to use girder.eventing.events instead?

[jbeezley]

For the package task, I think it would be appropriate to remove it. It is only there for publishing on pypi, which most people will never do. We could possibly put the equivalent command as a one liner npm script.

[jbeezley]

Are we punting on coverage for now, or is that still in the works?

[sebastienbarre]

@jbeezley oh sorry, no punting, I had forgotten to put it there, good catch. I need to do a quick experiment with SitePen/remap-istanbul: A tool for remapping Istanbul coverage via Source Maps (and gotwarlost/babel-coverage to some extent).

[jeffbaumes]

@sebastienbarre https://github.com/Kitware/candela also performs source-mapped coverage through Babel. If what we do can help inform your process (or vice-versa) it would be good to have a uniform solution.

[jbeezley]

Candela uses karma to handle those complexities. Porting existing tests to use karma would be a significant undertaking which is why I suspect @sebastienbarre has chosen a more direct approach.

[zachmullen]

Thanks for the write-up. My vote would be that we should get coverage working on this branch, then merge it and leave additional stuff to subsequent PR's.

[jeffbaumes]

Ah ok, thanks for the explanation @jbeezley.

[jbeezley]

There is also isparta for babel coverage if that helps.

[sebastienbarre]

Alright I'll write a quick remapper for the Blanket coverage files.
For example, if you consider js_coverage/user.cfg in the build tree (ellipsis mine):

Fclients/web/static/built/girder.app.min.js
...
L1 1
...
L23 167

Line 23 was hit 167 times in girder.app.min.js. Now open girder.app.min.js (ellipsis mine):

...
var View = __WEBPACK_IMPORTED_MODULE_1_backbone___default.a.View.extend({
   constructor: function constructor(opts) {
       // eslint-disable-line backbone/no-constructor
       if (opts && __WEBPACK_IMPORTED_MODULE_0_underscore___default.a.has(opts, 'parentView')) {
...

Line 23 is:

       if (opts && __WEBPACK_IMPORTED_MODULE_0_underscore___default.a.has(opts, 'parentView')) {

This piece of Javascript I just tested:

const sourceMap = require('source-map');
const fs = require('fs');

const SourceMapConsumer = sourceMap.SourceMapConsumer;

const checkSourceMap = filename => {
  const rawSourceMap = JSON.parse(fs.readFileSync(filename));
  const smc = new SourceMapConsumer(rawSourceMap);
  console.log(smc.originalPositionFor({
    line: 23,
    column: 1
  }));
};
checkSourceMap('girder/clients/web/static/built/girder.app.min.js.map');

will output:

{ 
  source: 'webpack:///clients/web/src/views/View.js',
  line: 9,
  column: 8,
  name: null 
}

now open View.js:

import _ from 'underscore';
import Backbone from 'backbone';

import events from 'girder/events';
import eventStream from 'girder/utilities/EventStream';

var View = Backbone.View.extend({
    constructor: function (opts) { // eslint-disable-line backbone/no-constructor
        if (opts && _.has(opts, 'parentView')) {

and line 9 is indeed:

        if (opts && _.has(opts, 'parentView')) {

Thanks mozilla/source-map

[zachmullen]

Great! Sounds like we are very close 😄

[sebastienbarre]

Pythoners? @zachmullen? Unfortunately I need to get going on my journey to NC. Could you guys take over please? :)

Two issues:

• Travis CI seems to think it's a good idea to fail the test js_coverage_combine_report that creates the Javascript coverage XML file for CDash because the code is not looking "good enough" (is it linting it at the same time? why oh why?),

 6: /home/travis/build/girder/girder/tests/js_coverage_tool.py:48: [C901] 'combine_report' is too complex (16)

• The remapping from blanket's .cvg coverage to the original source files using Webpack's source maps works "fine" on my machine:

env) girder-build$ /Users/Shared/Src/girder/env/bin/python2.7 "/Users/Shared/Src/girder/girder/tests/js_coverage_tool.py" "--threshold=75" "--source=/Users/Shared/Src/girder/girder" "--include-core" "combine_report" "/Users/Shared/Src/girder/girder-build/js_coverage"
Overall total: 20350 / 27032 (75.28%)

but the mattrobenolt/python-sourcemap package I'm using in tests/js_coverage_tool.py's combine_report has issues. Its lookup API expects a line and column in a destination file (say in girder.app.min.js), which are then remapped to a new line and column in a different (original) file (say in clients/web/src/views/layout/ProgressListView.js) using the source map (say girder.app.min.js.map). However, blanket only generates line numbers in its .cvg files (see girder_build/js_coverage/user.cvg for example). Even when calling the API with token = currentRemapper.lookup(line=lineNum, column=0) , sourcemap still throws IndexError exceptions where it shouldn't -- it already has the line number we need, but fails trying to compute a column number. We are not interested in the column number and that exception makes us miss a good amount of coverage.

What's the protocol?

• send a patch and create a PR to add an API to sourcemap that only accepts a line number?
• fork it, patch it, and use the fork?
• download it and include the source straight in the girder?

Thanks

[zachmullen]

I'm on it. See you soon!

On Sep 5, 2016 12:14 PM, "Sébastien Barré" notifications@github.com wrote:

Pythoners? @zachmullen https://github.com/zachmullen? Unfortunately I
need to get going on my journey to NC. Could you guys take over please? :)

Two issues:

• Travis CI seems to think it's a good idea to fail the test
  js_coverage_combine_report that creates the Javascript coverage XML
  file for CDash because the code is not looking "good enough" (is it linting
  it at the same time? why oh why?),

  6: /home/travis/build/girder/girder/tests/js_coverage_tool.py:48: [C901] 'combine_report' is too complex (16)

  • The remapping from blanket's .cvg coverage to the original source
    files using Webpack's source maps works "fine" on my machine:

env) girder-build$ /Users/Shared/Src/girder/env/bin/python2.7 "/Users/Shared/Src/girder/girder/tests/js_coverage_tool.py" "--threshold=75" "--source=/Users/Shared/Src/girder/girder" "--include-core" "combine_report" "/Users/Shared/Src/girder/girder-build/js_coverage"
Overall total: 20350 / 27032 (75.28%)

but the mattrobenolt/python-sourcemap
https://github.com/mattrobenolt/python-sourcemap package I'm using in
tests/js_coverage_tool.py's combine_report has issues. Its lookup API
https://github.com/mattrobenolt/python-sourcemap/blob/master/sourcemap/objects.py#L61
expects a line and column in a destination file (say in
girder.app.min.js), which are then remapped to a new line and column in a
different (original) file (say in clients/web/src/views/layout/
ProgressListView.js) using the source map (say girder.app.min.js.map).
However, blanket only generates line numbers in its .cvg files (see
girder_build/js_coverage/user.cvg for example). Even when calling the API
with token = currentRemapper.lookup(line=lineNum, column=0)
, sourcemap still throws IndexError exceptions where it shouldn't -- it
already has the line number we need, but fails trying to compute a column
number. We are not interested in the column number and that exception makes
us miss a good amount of coverage.

What's the protocol?

• send a patch and create a PR to add an API to sourcemap that only
  accepts a line number?
• fork it, patch it, and use the fork?
• download it and include the source straight in the girder?

Thanks

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1476 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAh7twEGckQcTVkOfgjAHTTtoimivPCLks5qnD_2gaJpZM4JR5aN
.

[sebastienbarre]

Thanks Zach. To help you a bit:

• you can generate a .cvg file inside girder_build/js_coverage/ by running any single test using ctest -R
• then run js_coverage_combine_report using ctest -R as well, this will create the js_coverage.xml file in the build tree.
• I've left two debug statements in the code; the second one, after the exception, is the problematic one, it is triggered because of sourcemap's issue with column numbers. The one immediately above it is of the same nature, but is not triggered if column=0 in the call to .lookup( (it would be if column=1, for example). Either statements would not be hit if there was an API remapping a line number to another line number.

Thanks!

[jbeezley]

Travis is only showing two problems. One is the static analysis (McCabe complexity) of the combining script. The other is the fontello issue I already fixed on master.

[zachmullen]

@jbeezley there are several browser tests failing also, it's just that CTest is segfaulting during the coverage aggregation step, so there is no submission to CDash for the browser build.

[zachmullen]

We are green, but we still have web client test failures on travis that are masked by CTest segfaulting. Anyone object if I go ahead and merge this into 2.0-integration and then fix our coverage and web client tests on a subsequent branch?

[jeffbaumes]

👏 👏

[jcfr]

🎆 🎉 🎈

[sebastienbarre]

So I'm curious. Why are we not sticking the Fontello files directly in the repo?

[jbeezley]

I considered it, but it's around a 0.5MB diff every time we want to add or remove an icon. I don't care much either way though.

[zachmullen]

I consider this a temporary solution until the network problems are resolved, or we find a different service entirely.