maxtaco / coffee-script

IcedCoffeeScript
http://maxtaco.github.com/coffee-script
MIT License
728 stars 58 forks source link

Inline runtime generation, fix test:browser #176

Closed zapu closed 8 years ago

zapu commented 8 years ago

Added inline runtime generation with build:inline_runtime task. The runtime is generated to extras/inline-runtime.js and is used in nodes.coffee to emit IcedRuntime, if the option is set to inline or window.

This method, however, limits compilation capabilities of iced as browser library. Iced in browser will not be able to emit inline runtime, because there will be no inline-runtime.js text file anywhere. Compiling with runtime: none and letting it use runtime from iced global variable will work - and that's what test:browser Cake task does right now.

All tests pass, for both test and test:browser. Keep in mind that test:browser runs less tests, grep testingBrowser to see which test files are skipped.

zapu commented 8 years ago

So what are we doing with this one?

zapu commented 8 years ago

About require('fs') in browser - I think you removed the comment while I was typing the reply. It might be useful anyways:

That's correct, this would be a limitation of the compiler in browser for now. It wouldn't be able to compile code with runtime set to 'inline'. I don't really see a good way around it.

I tried to make the compiler "decompile" it's own runtime and emit it, but it led to nowhere. I can try this again if it sounds like a reasonable solution to you.

maxtaco commented 8 years ago

Here's an idea I had:

inlineRuntime: (lefthand) ->
    inline_runtime = require('./inline-runtime).runtime
    "#{lefthand} = #{inline_runtime};"
zapu commented 8 years ago

OK, that was another idea that I've had before. I wasn't sure which one was better - decompiling or saving raw string. So I'll get to it later today.

zapu commented 8 years ago

Just for reference (and for other people interested, maybe), iced2 used to do this: https://github.com/maxtaco/coffee-script/blob/iced2/src/nodes.coffee#L3394 which was not ideal either.

maxtaco commented 8 years ago

(I editted my comment above, thank you @zapu!)

maxtaco commented 8 years ago

Indeed, the previous solution was bad for several reasons, especially because the code was different/worse and drifted further apart over time.

zapu commented 8 years ago

I added the commit, it's still pretty rough around the edges, but the tests pass and seems OK overall.

zapu commented 8 years ago

Changed this approach a little bit to be more lined up with your proposal, and removed irrelevant comments from this PR. Things are much cleaner right now.

I should also squash the commits before eventual merge.

maxtaco commented 8 years ago

Looking very good. Do you get all tests passing? I'm still seeing one failing (repl.coffee:69).

maxtaco commented 8 years ago
 node ./bin/cake test

failed 1 and passed 714 tests in 2.58 seconds 

  empty command evaluates to undefined 
  AssertionError: Expected undefined to equal iced> 
  at global.eq (/Users/max/src/iced/coffee-script/Cakefile:363:14)
  at /Users/max/src/iced/coffee-script/test/repl.coffee:69:3
  at Function.<anonymous> (/Users/max/src/iced/coffee-script/test/repl.coffee:38:17)
  at global.test (/Users/max/src/iced/coffee-script/Cakefile:287:12)
  at testRepl (/Users/max/src/iced/coffee-script/test/repl.coffee:38:3)
  at Object.<anonymous> (/Users/max/src/iced/coffee-script/test/repl.coffee:67:1)
  at Object.<anonymous> (/Users/max/src/iced/coffee-script/test/repl.coffee:1:1)
  at Module._compile (module.js:409:26)
  at Object.exports.run (/Users/max/src/iced/coffee-script/lib/coffee-script/coffee-script.js:136:23)
  at runTests (/Users/max/src/iced/coffee-script/Cakefile:412:22)
  at Object.action (/Users/max/src/iced/coffee-script/Cakefile:429:12)
  at helpers.extend.invoke (/Users/max/src/iced/coffee-script/lib/coffee-script/cake.js:44:26)
  at Object.exports.run (/Users/max/src/iced/coffee-script/lib/coffee-script/cake.js:70:20)
  at Object.<anonymous> (/Users/max/src/iced/coffee-script/bin/cake:7:38)
  at Module._compile (module.js:409:26)
  at Object.Module._extensions..js (module.js:416:10)
  at Module.load (module.js:343:32)
  at Function.Module._load (module.js:300:12)
  at Function.Module.runMain (module.js:441:10)
  at startup (node.js:134:18)
  at node.js:962:3

  function () {
      return fn(input, output, repl);
    }
zapu commented 8 years ago

Hm, there was a change in behavior in node but I'm no longer seeing that. I tried to make this change in upstream (https://github.com/jashkenas/coffeescript/pull/4120) but it wasn't accepted. What node version do you have? I'll try to find some time and dig through the changelogs.

Can you try something like this?

zapu@lab » node
> undefined
undefined
> console.log('hell')
hell
undefined
>
zapu@lab » node --version
v5.6.0

Also, what's with the versions? I guess their versioning changed after all the node.js/io.js things happened.

maxtaco commented 8 years ago

Ah, I'm running 4.3.2....

zapu commented 8 years ago

The correct way would be to detect repl settings (because I think you can change the undefined behavior) and make test check result according to this setting.

maxtaco commented 8 years ago

Well still broken for me on 5.10.1., but that's ok.

:+1: Ok, to merge!

zapu commented 8 years ago

Oh, cool! Thanks!

maxtaco commented 8 years ago

Thank you!