WIP: decaffeinate console_view plugin and set up Webpack build

[uglycoyote]

Work in progress, in conversation with @tardyp, @rodrigc.

• Decaffeinated coffeescript source files to turn them in to plain JS
• Set up Webpack build instead of gulp

To install and run

• npm install
• webpack (or webpack --watch to iterate on sources)

To test:
npm test

[codecov]

Codecov Report

Merging #4056 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4056   +/-   ##
=======================================
  Coverage   88.55%   88.55%           
=======================================
  Files         326      326           
  Lines       34330    34330           
=======================================
  Hits        30402    30402           
  Misses       3928     3928

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a14ff1...c7a3abf. Read the comment docs.

[tardyp]

I guess that one is not needed anymore as you ran decaffeinate

[tardyp]

There is still a bunch of unneeded returns we need to get rid of them

[uglycoyote]

Thanks, yes it does seem like the tests in particular end up with unnecessary returns in them. Since coffeescript has this property of returning without an explicit return statement, I guess the decaffeinate errs on the side of caution, re-adding return in many places where it's not needed. Ill take a look for those.

So far I haven't been able to get the test code running. Currently the package.json for this plugin doesn't include Angular as a dependency, and this causes the tests to fail to run. The plugin runs fine though because Angular is already loaded by the base www stuff before the plugin ever gets loaded. I'm not sure what the correct approach is to properly add the dependency on Angular but without bundling a duplicate copy of Angular into the webpack bundle.

[uglycoyote]

@tardyp @rodrigc FYI decaffeinate does seem quite cautious about assuming that the return value of a piece of code is needed. Here's a particularly egregious example of the kind of badness that happens when decaffeinate assumes that the return value of a function is needed.

original:

        @filtered_changes = []
        for ssid, change of @changesBySSID
            if change.comments
                change.subject = change.comments.split("\n")[0]
            for builder in change.builders
                if builder.builds.length > 0
                    @filtered_changes.push(change)
                    break

decaffeinated:

        this.filtered_changes = [];
        return (() => {
            const result = [];
            for (let ssid in this.changesBySSID) {
                change = this.changesBySSID[ssid];
                if (change.comments) {
                    change.subject = change.comments.split("\n")[0];
                }
                result.push((() => {
                    const result1 = [];
                    for (let builder of Array.from(change.builders)) {
                        if (builder.builds.length > 0) {
                            this.filtered_changes.push(change);
                            break;
                        } else {
                            result1.push(undefined);
                        }
                    }
                    return result1;
                })());
            }
            return result;
        })();

after manual cleanup

        this.filtered_changes = [];
        for (let ssid in this.changesBySSID) {
            change = this.changesBySSID[ssid];
            if (change.comments) {
                change.subject = change.comments.split("\n")[0];
            }
            for (let builder of Array.from(change.builders)) {
                if (builder.builds.length > 0) {
                    this.filtered_changes.push(change);
                    break;
                }
            }
        }

So when running decaf, be on the lookout for these return ( (()=>{...})() ); patterns. Fortunately, they seem easy to convert back to sanity by removing the outer pair of lines in this construct.

[uglycoyote]

@tardyp Maybe you can assist with what's needed in order to make Angular happy within the Karma test suite. I tried to follow some of what you did in the #3806 push, with adding angular and angular-mock to the package.json, and then putting imports for these in the test "entry point" file.

I'm currently failures because $state is null after running the code

    beforeEach(
        inject($injector => {
            $state = $injector.get('$state');
    }));

and also getting the error

	Error: [$injector:unpr] Unknown provider: $stateProvider <- $state

I Googled "angular stateProvider" and did not find much, so I assume it's a buildbot-specific thing and not a general Angular concept. Perhaps there's some things from www/base or guanlecoja that need to by imported / required into the test entrypoint as well so that webpack can bundle this stateProvider thing before it bundles our test code?

[uglycoyote]

Upon researching a bit further, I gathered that stateProvider is something that's part of a package called UI-Router. It seems like something that's being brought in from guanlecoja-ui, but it's not clear to me how (it does not appear in the package.json of guanlecoja-ui). Do you know a bit more about this, and how I would fulfill this dependency in the console_view so that tests can run?

[tardyp]

decaffeinated:
[ pile of crap ]

arg! what bothers me is that its not only decaffeinate, it is probably also the current generated code, which returns that.
I think that for this, it would be better to just fix the coffeescript code in the first place, and add explicit return statement at the end of the function.
Also, maybe the use of --loose might help to avoid the Array.from.

We should definitly run this before decaffeinate:

https://github.com/saifelse/coffeelint-no-implicit-returns

[tardyp]

Upon researching a bit further, I gathered that stateProvider is something that's part of a package called UI-Router. It seems like something that's being brought in from guanlecoja-ui, but it's not clear to me how (it does not appear in the package.json of guanlecoja-ui). Do you know a bit more about this, and how I would fulfill this dependency in the console_view so that tests can run?

guanlecoja-ui is fetched via bower. guanlecoja integrates bower directly, and the dependencies are described in guanlecoja/config.coffee . guanlecoja-ui comes with a vendor.js file, which already integrates some of the bower dependencies, including ui-router.

guanlecoja-ui is not available under npm, only on bower. In #4046 , I remove guanlecoja-ui from the dependency list, and rather include it directly in the main app.
So what you need to do is to include ui-router as a dev npm dependency, and then require it in your test bootstrap code. (test/main.js)

[uglycoyote]

@tardyp Tried adding ui-router directly to the package.json, and requiring it into the tests entry point file. sadly even though webpack is now bundling ui-router sources before our test code I'm still getting:

	Error: [$injector:unpr] Unknown provider: $stateProvider <- $state

(coming from the first few lines of describe("console view", function() {...)

I wonder if there's some special step that is missing where one needs to explicitly register the stateProvider with the injector? I would have thought that just including the ui-router sources would be enough.

Regarding the decaffeinate accidental returns, good idea -- trying to make sure there are explicit return statements in the coffeescript before running decaffeinate seems like the thing to do. Your fear is probably true that the regular coffeescript compiler would behave the same way. Since coffeescript does not require return statements, it would need to assume that whatever your last statement evaluates to should be returned by the compiled code. It's not like it can statically analyze your code and determine whether or not any of the callers care about the return value.

[uglycoyote]

I'm completely stalled out on this angular $stateProvider "unknown provider" issue. I have spent far more time trying to get past this one error than anything else on this PR. Not sure what else to try.

[tardyp]

As you don't have guanlecoja-ui, you should also get an error saying guanlecoja-ui is not found. This error should be triggered by a beforeEach(module("console_view")) in the test, which is supposed to instanciate the module console_view and its dependencies. We must publish a guanlecoja-ui on npm in the first place to make this work.

On Wed, Apr 25, 2018 at 6:47 AM uglycoyote ***@***.***> wrote: I'm completely stalled out on this angular $stateProvider "unknown provider" issue. I have spent far more time trying to get past this one error than anything else on this PR. Not sure what else to try. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4056 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGtI3OOYUsbSEvzDP-qzUDZxhprhgh3ks5tr__LgaJpZM4TeUPx> .

--

[uglycoyote]

@tardyp seems like getting the test working is blocked until guanlecoja-ui is on npm then? But I gathered from chatter on the IRC channel that this PR is feeling like a step in the right direction, so i can continue to chip away at it.

I was considering moving forward by doing the same process on the other plugins, which should be straightforward. Though I'm not sure what the best setup would be as far as the webpack config:

1. I could copy and modify the webpack.config.js that I currently have under console_view into each plugin directory. But that's not great, because then each plugin's webpack config needs to be maintained separately, when they should probably all be (mostly) the same
2. I could make a webpack.config.js in each plugin directory, factor out the common configuration into another .js file in the parent directory that each plugin could import and share, thus avoiding cut and paste.
3. I could make just one webpack.config.js that is for all of the plugins. Webpack supports multiple "entry points" so one webpack config can build several bundles (one per plugin). Then running webpack once would build all of the plugins. This might be the tidiest and most convenient option, but it also feels a bit odd since the plugins are all supposed to be separate independent entities that you can opt-in or opt-out of on a case-by-case basis. On the other hand, it's not a lot of code, so maybe it doesn't really matter if webpack is building a bundle for a plugin that you are not using?

Do you have any thoughts on the desired setup here? option #2 is probably most like what you have currently, where gulp is run separately in each plugin's directory but each plugin is sharing a common gulp configuration (brought in from the guanlecoja repo). Also if there's some common webpack config stuff, where should it go? New subdir underneath www?

Slightly off topic, but one annoyance i noticed with the current gulp plugin build is that user's browsers keep a cached copy and when I make edits to a plugin I keep needing to tell people to force-refresh their browser, a normal F5 or even a browser restart does not grab the new plugin bundle. The scripts.js containing the main part of the buildbot js code gets requested with a URL that says scripts.js?_1523050699222 where the ?_NNNN is some kind of a hash which updates when you build new code, preventing the cache from failing to get the new stuff. However the plugins don't have this unique hash, so browser cache holds onto the old version indefinitely.

Webpack supports putting a hash in the bundle name (see https://webpack.js.org/guides/caching/) though I haven't explored that. But if there's a trivial fix for this in the current gulp build then maybe that's worth doing in the meantime?

[tardyp]

@uglycoyote thanks for continuing efforts on this!
Would you like to work on pushing guanlecoja-ui and data-module to npm?

I think this is the next mandatory step which will unblock this efforts. I am lacking time to work on it and I don't want to be a blocker on this.

As per caching, indeed we have no cache busting methods for plugins. We do have one for main app though. For developement, I just open the developer console, and set disable cache.

Doing enabling sha1 in filename in webpack is a good option, but this needs to modify the main app to find which file to require instead of hardcoding scripts.js:
https://github.com/buildbot/buildbot/blob/master/www/base/src/app/layout.jade#L19

[uglycoyote]

@tardyp I could look into getting those on npm but I currently have no idea what that entails.

Regarding the cache busting... yes I guess it would be a problem to embed the hash in the name of the plugin bundle, because the "main app" is the thing generating the <script> tag to bring in the plugin bundle, and how would the main app know the hash?

Maybe the best solution is just to have the main app write the ?date() into the query for plugins as well. At least then, if you wanted to force everyone's browser to get the new versions of the plugins you could run gulp again on the main app to regenerate it with a new unique request.

For development, it's not an issue as force refreshing with Ctrl-F5 (or the checkbox you pointed out) bypasses the problem. I'm more worried about getting the updates out to other people on the team, because I have been making frequent fixes to our plugin and it's not really reasonable to expect everyone viewing buildbot to press Ctrl-F5 all the time or disable caching on their own browsers.

[uglycoyote]

@tardyp Getting guanlecoja-ui into NPM is waiting on confirmation from you about version number policy. Maybe it's not important, but I didn't want to screw it up by adding the wrong version number as I worried that might not be a change that is undoable once it's in the NPM registry. (see buildbot/guanlecoja-ui#13)

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

closing due to our stalled pull request policy

[uglycoyote]

@tardyp, @rodrigc Sorry for the stall out on this. Have had a busy period without much time to devote to this but I plan to revisit it later in the year when I may have a bit more time. I have been checking the IRC logs now and then, and have seen the webpack/typescript stuff mentioned once or twice. The most recent road blocks on this were that the tests were not working, which required getting some packages in to NPM.

I successfully added guanlecoja-ui package to NPM, and the data module was the other dependency. I had originally uploaded that to NPM as buildbot-data-js, because that's what the package.json says. However after we got in touch with Andras and got the permissions to the buildbot-data project I think the plan was to change the package.json to use buildbot-data as the name, update the buildbot-data on NPM, and then nuke the buildbot-data-js that I created so that there's not a confusing duplicate.

Is there any reason not to change the current name in the package.json to buildbot-data? I'm not sure what the story was on how it got to be called buildbot-data-js in there or if there's anything else relying on that, but I would assume changing it would work fine.

[tardyp]

Sounds good to me. Le jeu. 27 sept. 2018 à 22:58, uglycoyote <notifications@github.com> a écrit :

@tardyp <https://github.com/tardyp>, @rodrigc <https://github.com/rodrigc> Sorry for the stall out on this. Have had a busy period without much time to devote to this but I plan to revisit it later in the year when I may have a bit more time. I have been checking the IRC logs now and then, and have seen the webpack/typescript stuff mentioned once or twice. The most recent road blocks on this were that the tests were not working, which required getting some packages in to NPM. I successfully added guanlecoja-ui package to NPM, and the data module was the other dependency. I had originally uploaded that to NPM as buildbot-data-js <https://www.npmjs.com/package/buildbot-data-js>, because that's what the package.json <https://github.com/buildbot/buildbot/blob/master/www/data_module/package.json> says. However after we got in touch with Andras and got the permissions to the buildbot-data <https://www.npmjs.com/package/buildbot-data> project I think the plan was to change the package.json to use buildbot-data as the name, update the buildbot-data on NPM, and then nuke the buildbot-data-js that I created so that there's not a confusing duplicate. Is there any reason not to change the current name in the package.json <https://github.com/buildbot/buildbot/blob/master/www/data_module/package.json> to buildbot-data? I'm not sure what the story was on how it got to be called buildbot-data-js in there or if there's anything else relying on that, but I would assume changing it would work fine. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4056 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGtI0dL7tKuZC-MF28TFnEQHxuiikQCks5ufTvZgaJpZM4TeUPx> .

--