Bankai rewrite #241

yoshuawuyts · 2017-09-20T12:25:47Z

Everything is new! Would be cool if people could take this for a test ride so we can fix bugs. Thanks!

Bankai start

Bankai build

Known issues

SIGTERM can leave the cursor as hidden
extracting critical CSS does only detects classes, not tag name selectors
reloading scripts for SSR in document can crash, causing stale builds
SIPHASH24 sometimes generates a new hash for the same files (could be our browserify pipeline too, need to investigate)

goto-bus-stop · 2017-09-20T12:51:15Z

lib/http-server.js

+var os = require('os')
+
+var selfsigned = require('selfsigned')
+var getPort = require('getPort')


$ node ../../choojs/bankai/bin.js app.js module.js:529 throw err; ^ Error: Cannot find module 'getPort'

This require should be all lowercase getport for case sensitive file systems

Good catch; thanks!

YerkoPalma · 2017-09-20T13:00:22Z

Seems like tests are not passing for node 4

bendik · 2017-09-20T13:47:55Z

lib/reload-client.js

+
+  function handleScripts () {
+    log.info('scripts', 'reloading')
+    window.location.reload()


Reloading causes a SSE connection broken error in console, at least in Firefox.
How about adding this?

window.addEventListener('beforeunload', function (event) { source.close() })

Good call, fixed! :D

yoshuawuyts · 2017-09-20T14:57:37Z

Looks like this is caused by npm rather than Node versions. It seems that for whatever reason the documents:list event never fires; not sure why.

ajoslin · 2017-09-20T15:10:08Z

With this and @goto-bus-stop's work, Browserify is becoming the best choice again, like it used to be :)

Big thanks Yosh, I can't wait to try this out.

thlorenz · 2017-09-20T15:12:17Z

I may have the solution for your cursor problem. Wrote this a while back when I ran into similar problems. Not sure if it'll handle SIGTERM though (it has been a while).

yoshuawuyts · 2017-09-20T17:09:33Z

@thlorenz thanks for the pointer, fixed the cursor issue :D

jbergstroem · 2017-09-20T17:34:38Z

FWIW this doesn't work great with pnpm (npm replacement). Likely due to a missing dep and npm's flattening assumptions. I can't look into this right now -- expect tomorrow.

~/oss/bankai $ ./bin.js build -d example
14:30:17 ✨  created: ./dist
14:30:17 🚨  bankai.asset: could not find a file for 
14:30:17 ✨  created: ./dist/manifest.json
 
~/oss/bankai ±pr/241 $ echo $?
0

yoshuawuyts · 2017-09-20T17:38:21Z

@jbergstroem !!! that's great news; I thought it might be a missing file, but dependency-check didn't return any positives. Given this is true, it would fix it for node@4 too! :D

YerkoPalma · 2017-09-20T19:11:18Z

There are some options that now are not supported by bankai. Are they completly erased now? for instance the port and address options? Also there are differences from the api and the cli, for instance, how could you manipulate assets path from the cli? it seems that you can't but it can be done with the node api

jbergstroem · 2017-09-20T19:45:26Z

This is also a bit concerning imo (slow):

$ time node bin.js   
  Please specify a bankai command:
    $ bankai <command>

  For example:
    $ bankai start index.js

  Run bankai --help to see all options.
node bin.js  0.84s user 0.11s system 101% cpu 0.940 total

$ time npm help   
<snip>
npm help  0.19s user 0.07s system 76% cpu 0.337 total

yoshuawuyts · 2017-09-20T19:57:50Z

@YerkoPalma Ports are now automatically generated (8080-9000). The assets path has become more of a convention thing - picking up assets/ and content/. We haven't set extra paths yet for the JS API, but I think that'd be a good addition - although if you have some common paths in mind we could add those to the CLI too

@jbergstroem made the imports in bin.js dynamic; should be sped up by a whole bunch!

jbergstroem · 2017-09-20T20:11:55Z

I seem to be getting different hashing (clean repo). MacOS 10.13 17A362a, node 8.5.0:

not ok 34 ·· ·· <script src="/scripts/c4bc18653915ae8c/bundle.js" defer>
  ---
    operator: equal
    expected: '<script src="/scripts/c4bc18653915ae8c/bundle.js" defer>'
    actual:   '<script src="/scripts/75023dc608219a4e/bundle.js" defer>'
    at: assertHtml (/Users/jbergstroem/wrk/oss/bankai/node_modules/assert-html/index.js:82:15)

YerkoPalma · 2017-09-20T20:14:30Z

I'm cool with conventions, I just didn't see it in the readme so I had to dig into the code to find out that the cli use by default assets/ and content/ (to be added) as assets path. Maybe a changelog could be useful (another question, what about the electron mode?).

yoshuawuyts · 2017-09-20T20:29:46Z

@jbergstroem bizarre; that could only be if for some reason WASM is acting up, the input files have changed, or like the algo isn't working. Dep for that is in https://github.com/yoshuawuyts/buffer-graph/ and https://github.com/mafintosh/siphash24 - we might need to switch to blake2b tbh; will probably be more stable

edit: it definitely sounds as if the files have somehow changed; looked at the WASM function and it should be deterministic. Urgh, I'm not sure why the input files would change tho :/

jbergstroem · 2017-09-20T21:41:03Z

@yoshuawuyts you probs want this:

--- a/test/service-worker.js      2017-09-20 18:39:16.000000000 -0300
+++ b/test/service-worker.js  2017-09-20 18:39:11.000000000 -0300
@@ -47,7 +47,7 @@
     assert.ok(res.hash, 'output hash exists')
   })
 
-  compiler.script('bundle.js', function (err, res) {
+  compiler.scripts('bundle.js', function (err, res) {
     assert.error(err, 'no error writing script')
     rimraf.sync(tmpDirname)
   })
@@ -72,7 +72,7 @@
     assert.ok(res, 'output exists')
   })
 
-  compiler.script('bundle.js', function (err, res) {
+  compiler.scripts('bundle.js', function (err, res) {
     assert.error(err, 'no error writing script')
     rimraf.sync(tmpDirname)
   })

..and add it to index.js?

jbergstroem · 2017-09-20T21:59:16Z

@yoshuawuyts said:
it definitely sounds as if the files have somehow changed

At least the repo is intact:

$ rm -rf node_modules && npm i -q && git reset --hard && node test/document.js 

> [email protected] install /Users/jbergstroem/wrk/oss/bankai/node_modules/fsevents
> node install

[fsevents] Success: "/Users/jbergstroem/wrk/oss/bankai/node_modules/fsevents/lib/binding/Release/node-v57-darwin-x64/fse.node" already installed
Pass --update-binary to reinstall or --build-from-source to recompile
added 885 packages in 46.893s
HEAD is now at e6f1152 make it fast
<snip>
not ok 31 ·· ·· <script src="/scripts/c4bc18653915ae8c/bundle.js" defer>
  ---
    operator: equal
    expected: '<script src="/scripts/c4bc18653915ae8c/bundle.js" defer>'
    actual:   '<script src="/scripts/75023dc608219a4e/bundle.js" defer>'
    at: assertHtml (/Users/jbergstroem/wrk/oss/bankai/node_modules/assert-html/index.js:82:15)
    stack: |-
      Error: ·· ·· <script src="/scripts/c4bc18653915ae8c/bundle.js" defer>
          at Test.assert [as _assert] (/Users/jbergstroem/wrk/oss/bankai/node_modules/tape/lib/test.js:212:54)
          at Test.bound [as _assert] (/Users/jbergstroem/wrk/oss/bankai/node_modules/tape/lib/test.js:64:32)
          at Test.equal.Test.equals.Test.isEqual.Test.is.Test.strictEqual.Test.strictEquals (/Users/jbergstroem/wrk/oss/bankai/node_modules/tape/lib/test.js:347:10)
          at Test.bound [as equal] (/Users/jbergstroem/wrk/oss/bankai/node_modules/tape/lib/test.js:64:32)
          at assertHtml (/Users/jbergstroem/wrk/oss/bankai/node_modules/assert-html/index.js:82:15)
          at /Users/jbergstroem/wrk/oss/bankai/test/document.js:99:5
          at /Users/jbergstroem/wrk/oss/bankai/index.js:127:5
          at /Users/jbergstroem/wrk/oss/bankai/lib/queue.js:24:5
          at Array.forEach (<anonymous>)
          at Queue.ready (/Users/jbergstroem/wrk/oss/bankai/lib/queue.js:23:13)
  ...

s3ththompson · 2017-09-21T02:41:02Z

This looks amazing! So excited to swap it out in my current projects.

Would be nice if there was a quick JS API migration guide... should I just copy over the contents of cmd-start? Do you plan to leave handling gzip and content-type headers at the userland level?

yoshuawuyts · 2017-09-21T08:46:59Z

@s3ththompson oh boy; yeah so the whole API was rewritten from scratch. I'm not sure how much of a migration guide could exist, I'd mostly just suggest to read the docs.

That said, we should probably document the different directories we look for; same for files. Probably also document some of the flags that aren't documented yet (watch, reload), and errr - yeah that's about it? On it! 😁

sholtomaud · 2017-09-21T09:02:11Z

Can I suggest some little attention to #242 & #243 before pulling. Sorry. But many ★★★★★ for the direction.

yoshuawuyts · 2017-09-27T11:56:59Z

@jondashkyle thanks for pointing these out!

entry file: I can't think of a way around this at the moment. Even if we'd allow you to pass a --basedir flag to set a basedir, we'd run into trouble. At the moment the only way to approach this I can recommend would be to move index.js to the root of your project. Sorry :(
large files: yeah, was worried this might happen. Node only has, like 1.5GB of memory - anything beyond that would make it crash. You might want to try again, because we just moved off the WASM build (which might be what caused your crash to happen) and onto a regular JS one. The longer term solution for this will probably be to never copy the asset files into memory tho (especially with stuff like video, haha).
live reload: yeah, noticed that too. I think after this patch lands, and we get some of the other bugs fixed we should do a pass on performance. I'm pretty sure we can speed this up!

Hope this makes sense; thanks again for chiming in! ✨

yoshuawuyts · 2017-09-27T12:08:07Z

Hmmmmm, we definitely seem to be having some sort of race condition in our test somewhere. Something is throwing off the hash of bundle.js- but only sometimes

lrlna · 2017-09-27T12:13:39Z

http.js

+    var url = req.url
+    compiler.documents(url, function (err, node) {
+      if (err) {
+        return compiler.documents('/404', function (err, node) {


i was just looking this over trying to implement a default: you've got two err, the other one should probably be error or something

o/ Thanks for reporting - this is fine tho! The closures work alright in this case; there's no bugs here (:

yoshuawuyts · 2017-09-27T14:20:18Z

Soooo, all boxes are ticked for an initial beta release? Can people please verify this works on their code?

Biggest follow ups for next patches in the beta will be:

improve our error story. Right now if a crash happens, it can fail without warning which is super frustrating for people
figure out a better story for assets, so we don't overflow memory
perhaps sneak in stuff like pack-flat for browserify, if source maps get the fix
figure out pre-gyp on Windows for .br files; this one's tricky.

Holler if I've missed anything! Thanks all 😁

yoshuawuyts · 2017-09-27T16:27:35Z

TESTS ARE PASSING. SSR RELOADING WORKS. The time for final reviews is now. Unless something is totally broken for someone here, we should be good to merge and put out an initial beta release! 🎉

ajoslin · 2017-09-27T16:40:10Z

@yoshuawuyts errors are still not being outputted, both for bankai start and bankai build.

s3ththompson · 2017-09-27T17:18:12Z

@yoshuawuyts same wasm RangeError with blake2b (will open issue). can we start with a pure JS solution and upgrade to wasm when the kinks are worked out? i think there are still some subtle issues with wasm heap size that require some investigation...

FWIW, @jbergstroem's original PR worked for me with blakejs

jbergstroem · 2017-09-27T17:26:03Z

@yoshuawuyts sorry to say that hashing still seems to be a problem. I don't have too much time to dedicate to this but I'll make an attempt at debugging it soon.

Edit: I guess this doesn't have too big a significance seeing how its primary job is bundling assets, not consistently naming folders. I'd expect an issue to be opened though.

yoshuawuyts · 2017-09-28T13:36:11Z

@ajoslin you're totally right; hope this is better!

yoshuawuyts · 2017-09-28T15:39:05Z

Okay, so I'm not thrilled about the lastest pass on errors - but getting it 100% right means implementing proper progress bar logic, more control over re-rendering, and a partial rewrite of the way we create UI (might need to ditch ansi-diff-stream). It's a little over the top haha.

So kinda feel that for now error handling is good enough for a beta; going to fix the blake2b error and then we should be good!

yoshuawuyts · 2017-09-28T16:52:57Z

@s3ththompson reverted the change, range error should be fixed again!

time to release

Welp, that concludes this lil episode. Gonna merge this patch and publish as a beta release. Any feedback can be sent to #246. Thanks!

yoshuawuyts · 2017-09-28T16:56:37Z

📦 v9.0.0-rc1 ✨

s3ththompson · 2017-09-28T17:02:03Z

@yoshuawuyts RangeError fixed! thank you!

yoshuawuyts added 4 commits September 20, 2017 14:01

rewrite bankai

cf3ef20

prep for release

9663540

test on node 8

43a57ad

fix hash

5761fc0

goto-bus-stop reviewed Sep 20, 2017

View reviewed changes

bendik reviewed Sep 20, 2017

View reviewed changes

close SSE listener

2cee009

yoshuawuyts force-pushed the new-new-new branch from 681fb7f to 2cee009 Compare September 20, 2017 13:57

fix getport require

ac616e5

yoshuawuyts added 2 commits September 20, 2017 19:04

handle kill command

2901605

ah well, no node 4 beta

ff44e9a

make it fast

e6f1152

yoshuawuyts added 2 commits September 27, 2017 13:22

fix tests

e157c31

4 is out again

68fa4d7

lrlna reviewed Sep 27, 2017

View reviewed changes

yoshuawuyts added 2 commits September 27, 2017 14:22

fix bundle

5278cfa

invalidate cache on reload

5416de8

yoshuawuyts added 3 commits September 27, 2017 17:52

fix test regression

3f2abd7

fix reloading

986102d

fix tests?

f88788e

document http

d87c124

yoshuawuyts added 2 commits September 28, 2017 15:55

pretty errors

1a44644

fix arrow position

8aef434

yoshuawuyts force-pushed the new-new-new branch from 0976974 to 8aef434 Compare September 28, 2017 14:09

show errors during start

474d24d

yoshuawuyts mentioned this pull request Sep 28, 2017

use blakejs yoshuawuyts/buffer-graph#7

Merged

fix err not showing

a48dc15

yoshuawuyts merged commit 38df6f9 into master Sep 28, 2017

yoshuawuyts deleted the new-new-new branch September 28, 2017 16:53

s3ththompson mentioned this pull request Sep 29, 2017

Bankai v9 beta feedback #246

Open

Bankai rewrite #241

Bankai rewrite #241

Conversation

yoshuawuyts commented Sep 20, 2017 • edited Loading

Bankai start

Bankai build

Known issues

goto-bus-stop Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

yoshuawuyts Sep 20, 2017

Choose a reason for hiding this comment

YerkoPalma commented Sep 20, 2017

bendik Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

yoshuawuyts Sep 20, 2017

Choose a reason for hiding this comment

yoshuawuyts commented Sep 20, 2017 • edited Loading

ajoslin commented Sep 20, 2017 • edited Loading

thlorenz commented Sep 20, 2017

yoshuawuyts commented Sep 20, 2017

jbergstroem commented Sep 20, 2017

yoshuawuyts commented Sep 20, 2017

YerkoPalma commented Sep 20, 2017

jbergstroem commented Sep 20, 2017

yoshuawuyts commented Sep 20, 2017

jbergstroem commented Sep 20, 2017

YerkoPalma commented Sep 20, 2017

yoshuawuyts commented Sep 20, 2017 • edited Loading

jbergstroem commented Sep 20, 2017 • edited Loading

jbergstroem commented Sep 20, 2017

s3ththompson commented Sep 21, 2017

yoshuawuyts commented Sep 21, 2017

sholtomaud commented Sep 21, 2017

yoshuawuyts commented Sep 27, 2017

yoshuawuyts commented Sep 27, 2017

lrlna Sep 27, 2017

Choose a reason for hiding this comment

yoshuawuyts Sep 27, 2017

Choose a reason for hiding this comment

yoshuawuyts commented Sep 27, 2017 • edited Loading

yoshuawuyts commented Sep 27, 2017

ajoslin commented Sep 27, 2017

s3ththompson commented Sep 27, 2017 • edited Loading

jbergstroem commented Sep 27, 2017 • edited Loading

yoshuawuyts commented Sep 28, 2017

yoshuawuyts commented Sep 28, 2017

yoshuawuyts commented Sep 28, 2017 • edited Loading

time to release

yoshuawuyts commented Sep 28, 2017

s3ththompson commented Sep 28, 2017

yoshuawuyts commented Sep 20, 2017 •

edited

Loading

goto-bus-stop Sep 20, 2017 •

edited

Loading

bendik Sep 20, 2017 •

edited

Loading

yoshuawuyts commented Sep 20, 2017 •

edited

Loading

ajoslin commented Sep 20, 2017 •

edited

Loading

yoshuawuyts commented Sep 20, 2017 •

edited

Loading

jbergstroem commented Sep 20, 2017 •

edited

Loading

yoshuawuyts commented Sep 27, 2017 •

edited

Loading

s3ththompson commented Sep 27, 2017 •

edited

Loading

jbergstroem commented Sep 27, 2017 •

edited

Loading

yoshuawuyts commented Sep 28, 2017 •

edited

Loading