Closed apg closed 11 years ago
hey @apgwoz -
This performance improvement is very exciting!
One thing: the unit tests that I have do a good job of testing the syntax of the toffee language, but they're not sweet at testing a couple other things, like how browser support works (as it doesn't simulate a browser client). It's been on my todo list to make the run_tests fire up an actual express3 server and then use someone's client library to visit a page and make sure browser-side publishing works.
The reason I'm bringing this up: I see two things that I think don't work right that aren't in the language unit test.
These may be quick fixes for you.
Check out your branch, then:
cd test/express3/
coffee app.coffee
Then visit localhost:3033 and make sure you see 3 matching columns. (The middle and right will be green, showing they're matching the reference column.) With this change I see an error. I can help you hunt for this, but probably not till next week. Compare this to master and you'll see it usually works.
This may actually be the same bug as 1. I'm not sure.
Quick background: consolidate.js is a library by the guy who made express. It's a library to wrap around other templating languages, which in itself isn't that useful. But one thing it does is run unit tests on all the included templating languages to make sure they obey the same protocols for pubbing, which is kind of a good thing. With this toffee branch it gets an error:
toffee
✓ should support locals (292ms)
✓ should not cache by default (156ms)
✓ should support caching (35ms)
1) should support rendering a string
How I got this error:
make test
in that directory; toffee failed to passAnd then I compared:
make test
and it passedOf course, whatever makes it fail both these steps 1 and 2 should make it into a full test suite, which I can try to do at some point. In the meantime, I check these things manually when there's a big change to toffee.
@malgorithms I'll get these tests passing and let you know. I was sure there were other things that you did to verify. :)
@malgorithms these tests now pass. There were three little things:
hey @apgwoz -
I think re: num 3 they'd call this a feature of coffeescript. It was an annoyance for me somewhere else in the toffee project, too. I worked around it.
Cool that you have all this working.
This morning I got Toffee's testing framework improved. While I didn't get consolidate.js integrated into the test, I actually got browser testing working. now when you do this:
> cake build
> cake test
It does all the tests in run_tests which you're used to, including a new one which:
Do you want to rebase with my latest master and try the tests? If they pass, I can accept accept the pull request and have this live today, and I'll push a new version of toffee to npm with your speed improvements.
-cc
Hey @malgorithms,
I'll rebase now and let you know! Thanks!
On Mon, Feb 4, 2013 at 2:08 PM, Chris Coyne notifications@github.comwrote:
hey @apgwoz https://github.com/apgwoz -
I think re: num 3 they'd call this a feature of coffeescript. It was an annoyance for me somewhere else in the toffee project, too. I worked around it.
Cool that you have all this working.
This morning I got Toffee's testing framework improved. While I didn't get consolidate.js integrated into the test, I actually got browser testing working. now when you do this:
cake build cake test
It does all the tests in run_tests which you're used to, including a new one which:
- fires up an express3 server at localhost:3033
- visits the server (using a browser-simulating module called zombie), letting the JS run
- makes sure everything rendered correctly in the "browser"
- if failure, leaves server running, so you can visit it in your own browser
Do you want to rebase with my latest master and try the tests? If they pass, I can accept accept the pull request and have this live today, and I'll push a new version of toffee to npm with your speed improvements.
-cc
— Reply to this email directly or view it on GitHubhttps://github.com/malgorithms/toffee/pull/12#issuecomment-13093354.
Done and passing. cake test
is a much nicer experience, though it's a lot of new dependencies!
yeah, at least they're dev dependencies :-)
ok, working on the merge...
@apgwoz - voila, merged and npm published. toffee version 0.0.64 has your changes. thanks!
of course, you're the most active user of Toffee, so let me know if anything got funky as a result of these changes.
Yays!
Thanks! -- we'll let you know what gets funky, but hopefully nothing!
On Mon, Feb 4, 2013 at 2:58 PM, Chris Coyne notifications@github.comwrote:
@apgwoz https://github.com/apgwoz - voila, merged and npm published. toffee version 0.0.64 has your changes. thanks!
of course, you're the most active user of Toffee, so let me know if anything got funky as a result of these changes.
— Reply to this email directly or view it on GitHubhttps://github.com/malgorithms/toffee/pull/12#issuecomment-13095910.
As I mentioned in a personal email, this significantly speeds up rendering by compiling templates to functions rather than to scripts that must be executed in an execution context. I've tested this within the app we're using toffee for and am reasonably confident that I have not screwed up anything with these changes.
For timing comparison (I don't have real benchmark numbers, just the test output timing):
NEW
OLD
That's pretty significant, though the test suite isn't insanely complex. In our app, we seem to go from ~30ms to about ~10ms (for example), on my local machine. We make lots of use of partials and includes, which has been a huge pain point for us.