refactor: move compile step to first run of server #2889

EzraBrooks · 2017-11-24T17:55:05Z

Fixes #2884 in a less npm-dependent manner than my previous PR.
@dignifiedquire How do you feel about this solution?

EzraBrooks · 2017-11-24T18:58:33Z

Looks like this works, unless the CI jobs cache the static resources between builds. I assume I should squash these commits?

dignifiedquire · 2017-11-25T14:38:07Z

lib/server.js

+    var bundler = browserify(path.join(__dirname, '/../client/main.js'))
+    bundler.bundle().pipe(fs.createWriteStream(karmaJsPath)).once('finish', () => {
+      bundler = browserify(path.join(__dirname, '/../context/main.js'))
+      bundler.bundle().pipe(fs.createWriteStream(contextJsPath))


This creates a race condition, if the bundle is not generated fast enough, it will be missing when trying to be served. Need to make sure things are blocked to move on until it is finished.

dignifiedquire

Thank you, see comments for things that need adjusment

dignifiedquire · 2017-11-25T14:38:27Z

package.json

@@ -329,6 +329,7 @@
  "dependencies": {
    "bluebird": "^3.3.0",
    "body-parser": "^1.16.1",
+    "browserify": "^14.5.0",


Should be able to drop grunt-browserify as a depency now

EzraBrooks · 2017-11-25T16:37:11Z

Going to have to modify the server unit tests to handle the way it's now calling back to webServer.listen.

dignifiedquire · 2017-11-25T18:52:27Z

lib/server.js

+      bundler.bundle().pipe(fs.createWriteStream(karmaJsPath)).once('finish', () => {
+        bundler = browserify(path.join(__dirname, '/../context/main.js'))
+        // Start the web server once the last file has been browserified
+        bundler.bundle().pipe(fs.createWriteStream(contextJsPath)).once('finish', () => { startWebServer() })


no need to wrap into an arrow function

dignifiedquire · 2017-11-25T18:54:53Z

lib/server.js

+      self.log.info('Front-end scripts not present. Compiling...')
+      // Browserify the files asynchronously
+      var bundler = browserify(path.join(__dirname, '/../client/main.js'))
+      bundler.bundle().pipe(fs.createWriteStream(karmaJsPath)).once('finish', () => {


the two bundles should be created in parallel no need to wait for one or the other I think. Best you wrap bundle gen into a function that returns a promise and then use promise.all for the two invocations

dignifiedquire · 2017-11-25T18:55:17Z

lib/server.js

+    }
+
+    const karmaJsPath = path.join(__dirname, '/../static/karma.js')
+    const contextJsPath = path.join(__dirname, '/../static/context.js')


these can be pulled to the top of the file

dignifiedquire · 2017-11-25T18:55:44Z

lib/server.js

+    const karmaJsPath = path.join(__dirname, '/../static/karma.js')
+    const contextJsPath = path.join(__dirname, '/../static/context.js')
+    // Check if the static files haven't been compiled
+    if (!(fs.existsSync(karmaJsPath) && fs.existsSync(contextJsPath))) {


please use the async versions

fs.exists is deprecated, fs.existsSync is not. And it seems like it would be an unnecessary complication to use fs.stat on two separate files and make callbacks that && the results together.

dignifiedquire · 2017-11-25T20:00:54Z

lib/server.js

+      Promise.all([mainPromise, contextPromise]).then(() => {
+        startWebServer()
+      }, (error) => {
+        self.log.error(error)


please use catch to handle the errors

this needs to abort on error because if this fails it means we are unable to start properly

Should it just throw the error? Not sure what the correct way to trigger program exit is here.

I think exiting like it is done here: https://github.com/EzraBrooks/karma/blob/ba3da6824371c8824c6b5976b55048e54f0a9924/lib/server.js#L210-L212 should be the right course of action

EzraBrooks · 2017-11-25T21:23:14Z

What's the best course of action for making a test for this? File I/O has unpredictable execution times, so we should use a flag or event to detect - are we doing something similar elsewhere in the tests that could be mimicked?
Also, it seems like the set timeout on a single test is 2000ms, which might not necessarily be long enough to bundle & write the files.

dignifiedquire · 2017-11-26T13:52:16Z

You can increase the timeout by doing

it('is slow', function mytest () {
  this.timeout(5000)
})

For filesystem, we use a mock file system in other unit tests, e.g.

karma/test/unit/web-server.spec.js

Line 17 in 10fac07

_mocks['graceful-fs'] = mocks.fs.create({

EzraBrooks · 2017-11-26T16:48:11Z

Correct me if I'm wrong, but it seems like using a filesystem call makes more sense here than using a mock filesystem since we're looking for the actual existence of these files.

EzraBrooks · 2017-11-26T17:32:02Z

The Node 6 and 8 jobs on Travis crashed during their package installs for some reason.

EzraBrooks · 2017-11-27T16:21:49Z

@dignifiedquire Does this look good to you? Travis failed on conditions unrelated to the PR.

johnjbarton · 2017-11-27T17:09:32Z

The travis build fails with

[email protected]  invalid

That's pretty alarming ;-)

EzraBrooks · 2017-11-27T17:37:24Z

Ah, it failed differently this time.

Florian-R · 2017-11-27T17:46:45Z

@EzraBrooks Couldn't this be done in a way that don't slow down the first run everytime? Putting the burden of compiling client-side like this to every users for a niche use-case doesn't seems a very nice idea to me.

Also adding browserify as a deps mean adding a gigantic tree to node_modules, slowing further more the CI for consumers of Karma.

EzraBrooks · 2017-11-27T17:50:48Z

I don't think it's a niche use case to not have a private npm registry and want to work with a fork of a major repository like this. Additionally, it's not going to slow down your CI if you properly cache the static resources on your CI job. It checks for existence of the files - it doesn't just assume they don't exist.

EzraBrooks · 2017-11-27T18:58:16Z

I queued the Travis job back up and it ran fine. It had errored on an apt install previously for some reason.

dignifiedquire · 2017-12-05T10:04:53Z

Thank you

johnjbarton · 2018-01-16T22:22:18Z

If I understand correctly, this change was made to move a build step from post install to every run? Why? As was pointed out on #2884

NPM has a post-install step for this reason.

johnjbarton · 2018-01-16T22:43:44Z

Do we know specifically what problems occur with post install?

I think this PR optimizes for a less-used use-case, pull master from github, at the expense of the core use-case, npm install. Building master after pull is extremely common; yes it's an inconvenience but direct pull is not the common case.

EzraBrooks changed the title ~~refactor: move compile step to first run of server~~ [WIP] refactor: move compile step to first run of server Nov 24, 2017

dignifiedquire reviewed Nov 25, 2017

View reviewed changes

dignifiedquire requested changes Nov 25, 2017

View reviewed changes

dignifiedquire added the needs: feedback-from-author label Nov 26, 2017

EzraBrooks force-pushed the compile-static-resources-on-first-launch branch from 352c4df to e51aca1 Compare November 26, 2017 16:42

EzraBrooks added 2 commits November 26, 2017 11:44

refactor: move compile step to first run of server

b5d0690

test: Add test for first-run compile of static resources

e7433fc

EzraBrooks force-pushed the compile-static-resources-on-first-launch branch from e51aca1 to e7433fc Compare November 26, 2017 16:45

EzraBrooks changed the title ~~[WIP] refactor: move compile step to first run of server~~ refactor: move compile step to first run of server Nov 26, 2017

EzraBrooks removed the needs: feedback-from-author label Nov 27, 2017

dignifiedquire approved these changes Dec 5, 2017

View reviewed changes

dignifiedquire merged commit 78ad12f into karma-runner:master Dec 5, 2017

johnjbarton mentioned this pull request Jan 16, 2018

Adding browserify to the deps cause a huge cascade of new dependencies. #2921

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move compile step to first run of server #2889

refactor: move compile step to first run of server #2889

EzraBrooks commented Nov 24, 2017

EzraBrooks commented Nov 24, 2017

dignifiedquire Nov 25, 2017

dignifiedquire left a comment

dignifiedquire Nov 25, 2017

EzraBrooks commented Nov 25, 2017

dignifiedquire Nov 25, 2017

dignifiedquire Nov 25, 2017

dignifiedquire Nov 25, 2017

dignifiedquire Nov 25, 2017

EzraBrooks Nov 25, 2017

dignifiedquire Nov 25, 2017

EzraBrooks Nov 25, 2017

dignifiedquire Nov 25, 2017

EzraBrooks commented Nov 25, 2017

dignifiedquire commented Nov 26, 2017

EzraBrooks commented Nov 26, 2017

EzraBrooks commented Nov 26, 2017

EzraBrooks commented Nov 27, 2017

johnjbarton commented Nov 27, 2017

EzraBrooks commented Nov 27, 2017

Florian-R commented Nov 27, 2017 •

edited

Loading

EzraBrooks commented Nov 27, 2017

EzraBrooks commented Nov 27, 2017

dignifiedquire commented Dec 5, 2017

johnjbarton commented Jan 16, 2018

johnjbarton commented Jan 16, 2018

refactor: move compile step to first run of server #2889

refactor: move compile step to first run of server #2889

Conversation

EzraBrooks commented Nov 24, 2017

EzraBrooks commented Nov 24, 2017

Choose a reason for hiding this comment

dignifiedquire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EzraBrooks commented Nov 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EzraBrooks commented Nov 25, 2017

dignifiedquire commented Nov 26, 2017

EzraBrooks commented Nov 26, 2017

EzraBrooks commented Nov 26, 2017

EzraBrooks commented Nov 27, 2017

johnjbarton commented Nov 27, 2017

EzraBrooks commented Nov 27, 2017

Florian-R commented Nov 27, 2017 • edited Loading

EzraBrooks commented Nov 27, 2017

EzraBrooks commented Nov 27, 2017

dignifiedquire commented Dec 5, 2017

johnjbarton commented Jan 16, 2018

johnjbarton commented Jan 16, 2018

Florian-R commented Nov 27, 2017 •

edited

Loading