Coffeescript Update for Meteor 1.4

[mrmowgli]

This fixes minor issues with upgrades from 1.3.1 to 1.4 and brings the Todos coffeescript branch up to current with the master branch.

[apollo-cla]

@mrmowgli: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

[mrmowgli]

Hrmm Just using the tests from the main branch. I'll look into updating these, however I have seen some comments regarding testing kicking around. Anyone know what the current state of testing is for the todos app on the main branch?

[mrmowgli]

Tests run correctly locally, looking into test results from circle.

[mrmowgli]

Well this brings the coffeescript branch up to date, to 1.4, as well as the tests. Everything works, and should be at par with the current master branch. Please merge. Thanks!

[tmeasday]

Great stuff @mrmowgli! Thank you

[tmeasday]

@GeoffreyBooth have you seen this? It looks like you might be taking things in a similar direction?

[GeoffreyBooth]

I didn’t see this, but I’m not a fan of the backticked import/export. I initially created the branch to demonstrate how to use modules with require statements, and I still think that should be part of the example. We don’t need an example of how to use backticked code, they can just look at the ES2015 master for that.

I added a comment on meteor/meteor#7459 about how a bug was introduced in require sometime after 1.3. The CoffeeScript code that avoided circular references no longer works. I don’t think telling people to use backticks is a good solution, since those import and export statements are just getting transpiled down to require statements anyway. We should figure out why require isn’t working in the cases mentioned in that issue.

I also created https://github.com/meteor/todos/tree/coffeescript-1.4.1.1. There’s essentially no change between 1.3.4.4 and 1.4.1.1. I didn’t want to upgrade the main coffeescript branch to 1.4.1.1 because I wanted to keep current with whatever version master is on.

[GeoffreyBooth]

P.S. I’m working on a pull request for CoffeeScript that adds support for import and export. It’s hopefully nearly finished, in the testing phase at the moment. My next test is to try to swap out the coffee-script module that Meteor uses in its coffeescript package with my experimental version, and see if this patched version of Meteor can compile and run a version of the CoffeeScript Todos app that has unbackticked import and export statements. I’m trying to get Meteor running within a Docker container in order to do this, so that others can replicate the test without having to muck with the copy of Meteor installed on their machines, and I’m having issues; but I expect I’ll get it to work. (You should publish an official Meteor Docker image!) It’ll still take a fair bit of time before such a large addition to the CoffeeScript compiler has been tested by enough people to get pushed out onto NPM, but hopefully within a matter of weeks that can be a better solution.

Currently my PR doesn’t support non-top level import or export statements, like your import statements inside a if (Meteor.isServer) block, so that’ll be a further challenge (as you’re violating the spec with those). Unless we allow the spec to be violated without throwing an error, that will remain a case for require or backticks; so we really should figure out what went wrong with require post-1.3.

[mrmowgli]

I'm not a fan of the backticks either, however my main goal was a working version with the latest Meteor. I actually don't mind having an example somewhere of the backtick style, since it IS valid javascript. But I don't like having it in the TODO exaomple, since it's the main scaffolding. Hopefully the coffeescript pull request from @GeoffreyBooth will do the trick.

[GeoffreyBooth]

@mrmowgli we could maybe make a coffeescript-backticked-modules branch, to offer another example. Do you mind merging the current coffeescript branch into yours and resolving conflicts? Also you should remove references to ListsModule, TodosModule and incompleteCountDenormalizerModule, since those variables were workarounds to fix the circular-reference problem. Also please remove all debugging code (console.logs, etc.).

[GeoffreyBooth]

@tmeasday here’s the test I was referring to: https://github.com/GeoffreyBooth/coffeescript-modules-test-meteor-todos

It’s a fork of the Todos coffeescript branch rewritten to use unbackticked import and export statements, with the Meteor coffeescript package replaced by a fork that uses my updated CoffeeScript compiler.

[mrmowgli]

@GeoffreyBooth - I actually would rather avoid having any extra branches kicking around. I'd rather have there be one official way of dealing with this, and perhaps listing the backtick import as a temporary fix in the Readme. Of course that suggests the coffeescript fork gets pulled in. As far as merging goes I'm happy to merge any additional updates, and rip out what I can. I actually already pulled out the ListModules / TodosModules but left in the denormalizer, since I didn't understand why it was in there and wanted to keep the branch relatively close to your original submit. Hapy to rp out any more logging statements, although I think I have most of them out by now.

[GeoffreyBooth]

As to branches and whether backticks or require is the recommended way to handle modules in CoffeeScript, I defer to @tmeasday and MDG. I initially created this branch to demonstrate how to accomplish all the Meteor 1.3+ features in CoffeeScript without needing backticks; it covers classes and some other features, not just modules. The point was to provide a contrast with the master branch; if people want to know how to implement something in backticked ES2015, they can just look at master. The intention was never to present an MDG-approved example of how to write a Meteor app in CoffeeScript; MDG recommends ES2015, so I don’t think they have any preferences or recommendations for how to do anything in CoffeeScript.

That said, I’m not sure that’s MDG’s intention for this coffeescript branch, so if they want it to serve as a best-practices example and they think backticked import/export is preferred over require, that’s their call. Personally I don’t think one is better than the other, but I don’t think the community needs an example of backticked import/export.

I’ll happily rewrite my version if and when the modules PR gets merged into CoffeeScript, but I can’t imagine a change that large getting tested and accepted too quickly. We would also need to update the coffeescript Meteor package to use the new version of the coffee-script NPM module.

[tmeasday]

I think we are fairly unopinionated about CS, I'll leave it to you guys as to what should be "recommended" in this branch.

I would be inclined to agree with @GeoffreyBooth that less branches is probably better, as it's easier to keep them up to date and more discoverable for people.

[mrmowgli]

Hrmm trying to merge changes actually runs into some interesting issues, and from updating to 1.4.1.1 I think I see some hints of the meteor bug #7459 (See below).

So perhaps until the loader changes from @GeoffreyBooth get pulled in, we can hold off on this merge.

In the meantime anyone reading this thread should know that my branch (mrmowgli/todos) does work with Meteor 1.4.1, uses partial backticks for some imports, and is current with the main branch in terms of functionality and tests. You can use my fork as a working example against the latest versions of Meteor, until we get loader issues resolved.

And then it sounds like @GeoffreyBooth will be updating the original todos coffeescript branch for the new loader and bringing it current with master.

.meteor/packages/meteor-tool/.1.4.1_1.1twluyv++os.linux.x86_64+web.browser+web.cordova/mt-os.linux.x86_64/isopackets/ddp/npm/node_modules/meteor/promise/node_modules/meteor-promise/promise_server.js:165
      throw error;
      ^

TypeError: Cannot set property './todos.coffee.js' of undefined
    at ImportScanner._checkSourceAndTargetPaths (/tools/isobuild/import-scanner.js:275:7)
    at /tools/isobuild/import-scanner.js:178:12
    at Array.forEach (native)
    at ImportScanner.addInputFiles (/tools/isobuild/import-scanner.js:177:11)
    at /tools/isobuild/compiler-plugin.js:1011:15
    at Array.forEach (native)
    at Function.computeJsOutputFilesMap (/tools/isobuild/compiler-plugin.js:982:19)
    at ClientTarget._emitResources (/tools/isobuild/bundler.js:930:8)
    at /tools/isobuild/bundler.js:701:12
    at /tools/utils/buildmessage.js:359:18
    at [object Object].withValue (/tools/utils/fiber-helpers.js:89:14)
    at /tools/utils/buildmessage.js:352:34
    at [object Object].withValue (/tools/utils/fiber-helpers.js:89:14)
    at /tools/utils/buildmessage.js:350:23
    at [object Object].withValue (/tools/utils/fiber-helpers.js:89:14)
    at Object.enterJob (/tools/utils/buildmessage.js:324:26)
    at ClientTarget.make (/tools/isobuild/bundler.js:692:18)
    at /tools/isobuild/bundler.js:2586:14
    at /tools/isobuild/bundler.js:2675:20
    at Array.forEach (native)
    at Function._.each._.forEach (/home/alewis/.meteor/packages/meteor-tool/.1.4.1_1.1twluyv++os.linux.x86_64+web.browser+web.cordova/mt-os.linux.x86_64/dev_bundle/lib/node_modules/underscore/underscore.js:79:11)
    at /tools/isobuild/bundler.js:2674:7
    at /tools/utils/buildmessage.js:271:13
    at [object Object].withValue (/tools/utils/fiber-helpers.js:89:14)
    at /tools/utils/buildmessage.js:264:29
    at [object Object].withValue (/tools/utils/fiber-helpers.js:89:14)
    at /tools/utils/buildmessage.js:262:18
    at [object Object].withValue (/tools/utils/fiber-helpers.js:89:14)
    at /tools/utils/buildmessage.js:253:23
    at [object Object].withValue (/tools/utils/fiber-helpers.js:89:14)
    at Object.capture (/tools/utils/buildmessage.js:252:19)
    at bundle (/tools/isobuild/bundler.js:2567:31)
    at /tools/isobuild/bundler.js:2514:32
    at Object.withCache (/tools/fs/files.js:1585:12)
    at Object.exports.bundle (/tools/isobuild/bundler.js:2514:16)
    at /tools/runners/run-app.js:582:36
    at Function.run (/tools/tool-env/profile.js:489:12)
    at bundleApp (/tools/runners/run-app.js:572:34)
    at AppRunner._runOnce (/tools/runners/run-app.js:625:35)
    at AppRunner._fiber (/tools/runners/run-app.js:884:28)
    at /tools/runners/run-app.js:402:12

[GeoffreyBooth]

The current coffeescript branch is current with the version of Meteor used in master, currently 1.3.4.4. I also created a coffeescript-1.4.1.1 branch, which I’ll merge into coffeescript and delete whenever master gets updated to 1.4.1.1 or later. I don’t think the coffeescript branch should be ahead of master, that would be confusing to people.

Anyway I’m going to close this for now, as I don’t think we should be using backticks in a CoffeeScript example unless absolutely necessary. I’m pushing to get the modules PR accepted into CoffeeScript as soon as possible, so hopefully this will be a moot point before too long.

[hems]

any news?

[GeoffreyBooth]

I assume you’re asking about when a version of this with unbackticked import and export statements might be ready? The modules PR is here. It not only needs to get accepted, but then documentation written and a new version of CoffeeScript released onto NPM, and then the coffeescript Meteor package would need to be updated to use the new CoffeeScript module. So a version of this with import and export statements without backticks is still a ways off.

If you want to see a version of this with unbackticked import/export today, using the experimental CoffeeScript compiler hacked into Meteor’s coffeescript package, that’s here.

[GeoffreyBooth]

Well, that happened faster than I thought it would. CoffeeScript 1.11.0 with support for modules is here, and I’ve opened a PR to Meteor to update Meteor’s coffeescript package accordingly. When that PR is accepted, I’ll update the CoffeeScript Todos to use native import and export statements.