Inline runtime generation, fix test:browser #176

zapu · 2016-04-03T09:09:37Z

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.

Added Cakefile task to build inline runtime (iced-runtime-3) and save it to extras/inline-runtime.js. It is used in nodes.coffee when runtime: 'inline' is requested.

Improve IcedRuntime node to properly generate 'inline' and 'window' runtimes. It is done by including extras/inline-runtime.js file into the output. This file is generated with Cakefile build:inline_runtime task. Fix browser tests. When in test:browser task, test files will be compiled with runtime set to 'none'. `global.iced` will point to iced-runtime instead. This is done to simulate browser environment.

zapu · 2016-04-03T09:10:20Z

Cakefile

+
+  unless process.env.MINIFY is 'false'
+    {code} = require('uglify-js').minify code, fromString: true
+


Can someone check if uglifying works? It fails for me in both build:browser and build:inline_runtime with parse errors.

Yeah, no dice: mishoo/UglifyJS#448

Aww, so it's related to es6 features? Ok...

Yeah, seems to be puking on function*

Talking to @chrisnojima about this in the office, we probably shouldn't be outputting ES6 to browsers anyways. People should convert to ES6 via babel (or whatnot). This is of course a much deeper concern, and indicates a deeper change in workflow, so maybe we shouldn't spend too many cycles on this.

However, if we did, it still wouldn't change the fact that the actual code emittted from iced3 is in ES6 (with function*) and will break many browsers.

But this is the point of this branch, no? To emit ES6 for future browsers or as input for babel translation.

We can optionally support browserify in compiler - but do not actually specify it as a dependency - so it will simplify the workflow for people who have it installed anyways.

Agreed all around. Probably the most useful workflow is for us to output ES6, and then for consumers of the output code to minify and transpile as they see fit. Meaning, the whole direct browser operation of CoffeeScript might be moot for us, or more charitably, best-effort.

In sum, let's stick with the previous plan of (1) outputting the inline-runtime via raw string inclusion, and add (2) disabling of minification

Cool, thanks! I have (1) almost done but I need to write some tests. I'll try to have it all committed here at the end of the day.

zapu · 2016-04-06T06:54:16Z

So what are we doing with this one?

zapu · 2016-04-06T13:11:34Z

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 · 2016-04-06T13:12:21Z

src/nodes.coffee

@@ -2437,6 +2438,10 @@ class IcedRuntime extends Block
    if @isEmpty() then []
    else               super o

+  inlineRuntime: (lefthand) ->
+    fs = require('fs')


My only frustration is that we used to have something that did work here. So how do you think the browser compiler should work then --- the user has to insert their own runtime? I'm getting a little confused.

maxtaco · 2016-04-06T13:15:56Z

Here's an idea I had:

In the cake build:inline-runtime step, output a lib/coffee-script/inline-runtime.js file that has the code from inline-runtime.js in a raw string
Emit the raw string on compilation in inline mode, instead of reading the file off of the fs via require('fs'). So instead, something of the form:

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

zapu · 2016-04-06T13:17:39Z

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 · 2016-04-06T13:19:06Z

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 · 2016-04-06T13:19:12Z

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

maxtaco · 2016-04-06T13:19:58Z

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

Inline runtime will now contain stringified copy of itself. This is done in order to be able to emit the runtime from a browser-based compiler (or any environment that does not have a filesystem or access to compiler files). Removed uglifyjs support as it did not work with ES6 code. Renamed "build:inline_runtime" task to "build:inline-runtime"

zapu · 2016-04-07T10:04:30Z

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

Stringified inline runtime is now accessible via require('./inline-runtime-str') from both node version and the browser. Much cleaner solution than the previous one.

zapu · 2016-04-07T11:59:44Z

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 · 2016-04-08T20:52:22Z

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

maxtaco · 2016-04-08T20:52:52Z

 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 · 2016-04-08T21:08:33Z

Hm, there was a change in behavior in node but I'm no longer seeing that. I tried to make this change in upstream (jashkenas#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 · 2016-04-08T21:16:25Z

Ah, I'm running 4.3.2....

zapu · 2016-04-08T21:26:23Z

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 · 2016-04-08T21:37:33Z

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

👍 Ok, to merge!

zapu · 2016-04-08T21:40:19Z

Oh, cool! Thanks!

maxtaco · 2016-04-08T21:41:19Z

Thank you!

zapu added 2 commits February 13, 2016 21:08

Compile inline runtime to inline-runtime.js

91d763f

Added Cakefile task to build inline runtime (iced-runtime-3) and save it to extras/inline-runtime.js. It is used in nodes.coffee when runtime: 'inline' is requested.

zapu reviewed Apr 3, 2016
View reviewed changes

zapu mentioned this pull request Apr 3, 2016

InlineRuntime no longer works #173

Closed

maxtaco reviewed Apr 6, 2016
View reviewed changes

zapu added 2 commits April 7, 2016 11:02

call handleIcedOptions in command.coffee

437b83a

Save inline runtime in lib/ instead of extras/

5ea0417

Stringified inline runtime is now accessible via require('./inline-runtime-str') from both node version and the browser. Much cleaner solution than the previous one.

maxtaco merged commit 5e1d887 into maxtaco:iced3 Apr 8, 2016

zapu deleted the extras_inline_runtime branch May 12, 2018 10:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inline runtime generation, fix test:browser #176

Inline runtime generation, fix test:browser #176

zapu commented Apr 3, 2016

zapu Apr 3, 2016

maxtaco Apr 6, 2016

zapu Apr 6, 2016

maxtaco Apr 6, 2016

maxtaco Apr 6, 2016

maxtaco Apr 6, 2016

zapu Apr 6, 2016

maxtaco Apr 6, 2016

maxtaco Apr 6, 2016

zapu Apr 6, 2016

zapu commented Apr 6, 2016

zapu commented Apr 6, 2016

maxtaco Apr 6, 2016

maxtaco commented Apr 6, 2016

zapu commented Apr 6, 2016

zapu commented Apr 6, 2016

maxtaco commented Apr 6, 2016

maxtaco commented Apr 6, 2016

zapu commented Apr 7, 2016

zapu commented Apr 7, 2016

maxtaco commented Apr 8, 2016

maxtaco commented Apr 8, 2016

zapu commented Apr 8, 2016

maxtaco commented Apr 8, 2016

zapu commented Apr 8, 2016

maxtaco commented Apr 8, 2016

zapu commented Apr 8, 2016

maxtaco commented Apr 8, 2016


		unless process.env.MINIFY is 'false'
		{code} = require('uglify-js').minify code, fromString: true

Inline runtime generation, fix test:browser #176

Inline runtime generation, fix test:browser #176

Conversation

zapu commented Apr 3, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zapu commented Apr 6, 2016

zapu commented Apr 6, 2016

Choose a reason for hiding this comment

maxtaco commented Apr 6, 2016

zapu commented Apr 6, 2016

zapu commented Apr 6, 2016

maxtaco commented Apr 6, 2016

maxtaco commented Apr 6, 2016

zapu commented Apr 7, 2016

zapu commented Apr 7, 2016

maxtaco commented Apr 8, 2016

maxtaco commented Apr 8, 2016

zapu commented Apr 8, 2016

maxtaco commented Apr 8, 2016

zapu commented Apr 8, 2016

maxtaco commented Apr 8, 2016

zapu commented Apr 8, 2016

maxtaco commented Apr 8, 2016