Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Faster, simpler and more robust debug/bench build setup #3019

Merged
merged 16 commits into from Aug 18, 2016
Merged

Conversation

mourner
Copy link
Member

@mourner mourner commented Aug 17, 2016

Closes #3014.

Greatly improves our debug and benchmark setup by using a set of NPM scripts that rely on watchify and st, removing the custom server.js script with express and browserify-middleware.

Introduces 3 independent server scripts:

  • npm run start-debug watches gl-js code and runs a static server for debug pages in parallel; you access them through localhost:9966/debug/
  • npm run start-bench downloads bench data, watches all benchmark-related code and runs a static server; you access benchmarks through localhost:9966/bench/; append #fps to run the FPS benchmark, etc.
  • npm start combines the two above

With this setup, any code change is updated on the pages in 200-300ms instead of 20 seconds, making GL JS development pleasant again. It's also much simpler and more reliable.

@anandthakker
Copy link
Contributor

@mourner I ❤️ this change!

My only comment is that it might be worth considering budo (a lightweight dev server that wraps watchify) instead of the homegrown watchify/st combo. I am pretty sure it would allow exactly the same use cases, but with fewer helper npm scripts.

@mourner
Copy link
Member Author

mourner commented Aug 17, 2016

@anandthakker we used Budo before, but @lucaswoj dropped it as a part of #2231. I still prefer watchify + static server setup because it's way simpler, more flexible, and we don't really need live reload IMO.

@mourner
Copy link
Member Author

mourner commented Aug 17, 2016

It's worth noting that the main reason I split the servers back is that the debug server becomes ultra-fast — it starts from scratch in just a few seconds (the first watchify build takes ~1.7s). The benchmark server, which is needed less often, starts up with a build in ~15s.

However with this setup, I can actually change the scripts so that npm start sets up both debug and bench servers, and then have npm run start-debug and npm run start-bench if I want to run them separately. edit: did this, it's glorious!

@anandthakker
Copy link
Contributor

I still prefer watchify + static server setup because it's way simpler, more flexible, and we don't really need live reload IMO.

I'm sold 👍

"build-token": "browserify debug/access-token-src.js --debug -t envify > debug/access-token.js",
"watch-bench": "watchify bench/index.js --plugin [minifyify --no-map] -t [babelify --presets react] -t unassertify -t envify -o bench/bench.js -v",
"download-bench-data": "node bench/download-data.js",
"start-server": "st --no-cache --localhost --port 9966 --index index.html .",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mourner is there an easy way to increment the port until an open one is found? Asking because budo by default uses the same one, and I've definitely run into collisions while developing on gl-js.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know of an easy way but I could do a PR to st or make a wrapper in a Node script.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened an st issue isaacs/st#81, will follow-up with a PR if the idea is approved.

@lucaswoj
Copy link
Contributor

lucaswoj commented Aug 17, 2016

This looks great! I love how this simplifies our debug server into small, sharp, and composable tools.

Mixing generated and source files in the same directory adds clutter and is potentially confusing (e.g. bench/index.js vs bench/bench.js). What do you think about using a different directory for generated files, using to budo to generate these files, or trying to fix the original browserify-middleware bug?

The previous URL structure was cleaner. I appreciate that it's tricky to recreate without using a more sophisticated server like express. 💭

@mourner
Copy link
Member Author

mourner commented Aug 17, 2016

I agree about generated files — would it suffice to move bench/bench.js into bench/bundle/bench.js and debug/access-token.js into debug/bundle/access-token.js? Another option would be to create a thin wrapper script around st that would generate those on the fly, but the first is simpler.

What are your concerns about the new url structure? I find it cleaner than the previous one because it's explicit and predictable, without "magic" — for every referenced URL, it's clear where it comes from.

The original browserify-middleware issue is more of a design decision than a bug — it just isn't designed to handle cases like ours well, in addition to not being maintained anymore. So I'd avoid it if possible. I'm also hesitant about Budo because it's not suited for multiple target use case, and also seems like an overkill.

@mourner
Copy link
Member Author

mourner commented Aug 17, 2016

Just to be clear, I'd like to land this PR as soon as possible because the current situation is severely crippling my ability to work on GL JS productively, and it's hard to appreciate the elegance of the URL design when every page request takes half a minute. We can flesh out the nuances in follow-up PRs.

@mourner
Copy link
Member Author

mourner commented Aug 18, 2016

Switched to npm-run-all for Windows compatibility and simper parallel NPM scripts, and I think this looks pretty awesome: 203ab48

@lucaswoj lucaswoj merged commit 9fd707a into master Aug 18, 2016
@lucaswoj lucaswoj deleted the watchify branch August 18, 2016 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants