Closed mrmowgli closed 8 years ago
@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/
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?
Tests run correctly locally, looking into test results from circle.
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!
Great stuff @mrmowgli! Thank you
@GeoffreyBooth have you seen this? It looks like you might be taking things in a similar direction?
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 https://github.com/meteor/meteor/issues/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.
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.
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.
@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.log
s, etc.).
@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.
@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.
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.
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.
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
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.
any news?
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.
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.
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.