Upgrade to v2 of coffee

brucejo75 commented 6 years ago

Make meteor-file-collection work with V2 of Coffescript (specifically 2.0.3.3).

Changes to `package.js`

I make the project dependent upon coffeescript@2.0.3_3.
I reset the version of vsivsi:meteor-file-collection to 2.0.0. Because of the dependency on coffeescript@2.0.3_3.

I believe all changes are backward compatible with v1 of CoffeeScript. But I did not retest them.

3 areas needed to be addressed:

`super` and `this`

Needed to refactor the FileCollection class constructor for both client and server.

`FileCollection.super` does not exist

Simply set FileCollection.__super__ to Mongo.Collection.prototype

String interpolations behave differently

In v1 a string interpolation for an ObjectID would resolve to the string portion of the object e.g. the _str property. This is no longer the case with v2.

vsivsi commented 6 years ago

Thanks for getting this started @brucejo75! Do you intend to keep going with the three areas listed above? I'll be happy to merge and push a release once everything looks ready.

brucejo75 commented 6 years ago

Thanks @vsiverson!

Yeah, I did those fixes in the pr and it all works now for my project in v1.6.1.

I am trying to find the errors in the tests. I fixed one.

Working on the resumable.js issues now. I suspect I have one error that is messing up all of the tests....

Slow going, I am Coffeescript illiterate and resumable.js just looks like a big hunk of stuff...

BTW: what do you think about changing the version to 2.0.0? I thought it was the only way to go because CS 1 & 2 are incompatible.

brucejo75 commented 6 years ago

Hey @vsiverson, can you point me to where the Basic resumable.js ... tests are poking file-collection?

I am just dropping on console.log statements in functions that look like they are the ones.

Or better yet, how can I run a debugger with spacejam? meteor test-packages starts up but the web interface (localhost:3000) is not connecting.

vsivsi commented 6 years ago

@brucejo75 Hey, thanks for working on this today. Unfortunately I'm busy for the rest of the day, I'll try to take a look at what you've done over the weekend and try to answer your questions above. I'm pretty rusty on this code myself at this point. Reminder to self, I should probably look into updating resumable.js as well in this release.

Resumable is definitely a messy codebase, and there is a fork that seems to be better maintained (name is escaping me), but I'm probably not going to put in the work to manage a probably breaking change like that.

brucejo75 commented 6 years ago

Hi @vsiverson, can you point me to where to look? I will be happy to run this down. Thanks!

vsivsi commented 6 years ago

Okay, I took a quick look. The tests only seem to be failing on the server-side which is curious, the same tests run from the client are all working. It's not immediately clear to me what would cause that...

Note that resumable.js is not actually being used in any of these failing tests. These are tests of the backend API that resumable.js uses, but the tests themselves are simple and mock the library. So there's no need to be digging around in resumable.js itself.

The resumable.js mock is:
https://github.com/vsivsi/meteor-file-collection/blob/master/test/file_collection_tests.coffee#L465-L524

The failing tests start here (again, failing on the server-side only):
https://github.com/vsivsi/meteor-file-collection/blob/master/test/file_collection_tests.coffee#L534

Not sure if that helps you out much or not. I really haven't had time to come up to speed on the differences in CoffeeScript2 because my motivation to do so is low, given that I do not currently plan to use Coffeescript (1 or 2) in any future projects.

vsivsi commented 6 years ago

I just tried running off your branch and I am seeing the same errors on the server-side when I run meteor test-packages (the web-UI is running for me). What is strange is the vanilla HTTP API endpoints are working fine, and no errors are appearing in the console, so something is subtly wrong with the tests, but only when run on the server.... Hmmm

edemaine commented 6 years ago

Just looking quickly: https://github.com/vsivsi/meteor-file-collection/blob/master/test/file_collection_tests.coffee#L465-L524 has some string interpolations of _id that probably need the ._str suffix as in the rest of this PR.

brucejo75 commented 6 years ago

Thanks @vsiverson for your response. Thanks @edemaine for identifying the issues!

@vsiverson, all tests now pass. Ready to roll!

vsivsi commented 6 years ago

Great news!

Okay, so I'm super annoyed that for any version after Meteor 1.6, the localhost:3000 interface for meteor test-packages doesn't reliably run the tests anymore. I had it working last night, but now I can't get it to work again.

Some other TODOs before release...

[x] Make the damn meteor test-packages web interface reliable again.
[ ] Update all of the Atmosphere packages to latest versions
[ ] Update some npm packages, as makes sense
[ ] Update included version of resumable.js
[ ] Make sure everything still works with above updates
[ ] Test that the file-collection sample app works correctly with Meteor 1.6 and the updated file-collection.
[ ] Coordinate release of updated file-collection with updated job-collection, and ensure that both work correctly with the combined sample app

Also, @brucejo75 mentioned a major version bump to 2.0... Something like that is probably necessary. But here's an idea to ponder....

Some time ago I created a new Github Org called "Darkmattergy" (like Dark Matter + Energy, in keeping with the Meteor cosmic theme). I'm considering transferring file-collection and job-collection to that org on github, and setting up matching orgs on Atmosphere/NPM so that these projects and their associated bits can live on without my direct involvement. The "vsivsi" versions will deprecate with their Meteor 1.5.x support intact, and for Meteor 1.6+, everything will switch over to the new Org.

The job-collection package is a lot more popular than this one, and a similar effort to port it over with minimal involvement from me has happened there, so these are both pretty close to being ready to release.

Anyway, this seems like the best moment to make such a move if it is ever going to happen. We have breaking changes in both these packages and the Meteor + Coffeescript platform. There's a clear boundary here between what can be deprecated and what can be supported moving forward etc.

What do you guys think? Are you willing to be founding members of this new org (with the privileges and responsibilities that may entail?)

vsivsi commented 6 years ago

BTW, here are all of the latest versions of the dependent packages...

Package.onUse(function(api) {
  api.use('coffeescript@2.2.1_1', ['server','client']);
  api.use('webapp@1.5.0', 'server');
  api.use('mongo@1.4.5', ['server', 'client']);
  api.use('minimongo@1.4.3', 'server');
  api.use('check@1.3.0', ['server', 'client']);
  api.addFiles('resumable/resumable.js', 'client');
  api.addFiles('src/gridFS.coffee', ['server','client']);
  api.addFiles('src/server_shared.coffee', 'server');
  api.addFiles('src/gridFS_server.coffee', 'server');
  api.addFiles('src/resumable_server.coffee', 'server');
  api.addFiles('src/http_access_server.coffee', 'server');
  api.addFiles('src/resumable_client.coffee', 'client');
  api.addFiles('src/gridFS_client.coffee', 'client');
  api.export('FileCollection');
});

Package.onTest(function (api) {
  api.use('vsivsi:file-collection@' + currentVersion, ['server', 'client']);
  api.use('coffeescript@2.2.1_1', ['server', 'client']);
  api.use('tinytest@1.1.0', ['server', 'client']);
  api.use('test-helpers@1.0.11', ['server','client']);
  api.use('http@1.4.0', ['server','client']);
  api.use('ejson@1.1.0',['server','client']);
  api.use('mongo@1.4.5', ['server', 'client']);
  api.use('check@1.3.0', ['server', 'client']);
  api.use('tracker@1.1.3', 'client');
  api.addFiles('test/file_collection_tests.coffee', ['server', 'client']);
});

vsivsi commented 6 years ago

It's been so long, I somehow forgot that when running tests in a package directory you need to specify the CWD to the command: meteor test-packages ./. Doh.

brucejo75 commented 6 years ago

To be honest, my plan is to switch over to using fetch and formData or something like it. Along with something like node-fetch on the server.

I adopted File-Collection when I was first starting to use Meteor (prior to v1.3 and npm support).

For my application File-Collection is really overkill for what I am looking for and need. Plus I want to get my file data out of Mongo.

My goal with this PR was simply to get it running with Meteor 1.6.1.

Also, I know nothing about coffeescript. I do not understand the syntax and I think the whole concept is rife with issues (like adding additional dependency complexity that necessitated this PR).

I think your plan is a good one and the right thing to do. And I would be happy to help a little bit to get over the hump. But for my purposes it simply an exercise in doing the simplest thing for my app (make it work with Meteor v1.6.1 and not rewrite for fetch and formData yet.

edemaine commented 6 years ago

Sad to see people leaving both CoffeeScript and Meteor! I am still an active user of both, and of this package.

I'm willing to help (some) with this package (e.g. I still want to add proper mobile support, just haven't gotten to it yet), but less so with the entire organization.

vsivsi commented 6 years ago

I haven’t stopped liking CoffeeScript, it was a great tool for its time. But in the ES6+ world, it’s just less necessary, and with the rise of more complex JS build toolchains it just adds one more source of potential issues and maintenance (this PR being exemplary!) Plus TypeScript has a lot of merits for larger projects, but those merits are incompatible with the remaining benefits of CoffeeScript.

I’m also not down on Meteor per se, my work just no longer requires it. I don’t think it has in any way out-lived it’s usefulness. If anything, I think it has only recently stabilized to the point where I’d feel comfortable building something “real” with it. I do fear what happens if/when the MDG transitions to the next thing though. Meteor has a lot of moving parts, and that complexity will be difficult to maintain on a volunteer basis.

Anyway, me pulling away from this is about my situation, not a mark against these techs.

apendua commented 6 years ago

@vsivsi @brucejo75 Is there a chance this PR will be merged soon? Thanks for all the work!

antoninadert commented 6 years ago

Sorry to jump in, but what is lacking for this to be merged? By the way, I noticed what is probably a typo in the pull request (unless I am wrong, please tell)

I can't wait to use this project in Meteor 1.6, please tell us

brucejo75 commented 6 years ago

thanks @antoninadert, I just pushed a fix for that one.

hluz commented 6 years ago

Any chance of merging this soon?

juliomac commented 6 years ago

Also looking forward to see this merged!

thumptech commented 5 years ago

When merge?

antoninadert commented 5 years ago

I think we need a new README to explain which version works with which version of Meteor and then merge.

@brucejo75 What do you think?

edemaine commented 5 years ago

FWIW, I just published edemaine:file-collection on Atmosphere. It's almost exactly the current release of vsivsi:file-collection, but with the CoffeeScript code compiled to JavaScript via CoffeeScript v1 (avoiding the need for this PR), and jumping through a few hoops to make it all work; see Github link. This means you can drop it in as a replacement and upgrade your Meteor apps, until this PR gets finished/merged/released. The tests all pass, and I've successfully upgraded my own Meteor app.

brucejo75 commented 5 years ago

Thanks @edemaine,

Do you have any interest in taking over this project for @vsiverson, he has a list of todos before he was willing to take the PR. And he is looking for someone to own this project and I am not the right person.

edemaine commented 5 years ago

@brucejo75 @vsiverson Yes, I'm willing to help maintain file-collection. It sounds like this PR is done, just needs another review; and then the other to-do items could be turned into issues and addressed by future PRs before release.

vsivsi commented 4 years ago

@edemaine @brucejo75 Any interest in maintaining this package if it is transitioned to the Meteor Community Packages organization?

See: https://github.com/Meteor-Community-Packages

My other main meteor package, job-collection, looks to be moved over there, and I'd like to do the same for this one, assuming people are still using it, as I'm no longer an active Meteor developer.

See these discussions for more info: https://github.com/Meteor-Community-Packages/organization/issues/30 https://github.com/vsivsi/meteor-job-collection/pull/268

edemaine commented 4 years ago

@vsiverson Yep. I can try to review PRs at least, and I do have some old plans to add mobile support eventually (which we discussed long ago). Should we open a new issue on https://github.com/Meteor-Community-Packages/organization/issues/?

vsivsi commented 4 years ago

I think so. I'm not sure what their workflow is over there. You might ask @mitar for guidance if it's not clear, as he seems to be spearheading the effort for job-collection. Given the shared history of these two packages, it might be smoother to try to do them together.

mitar commented 4 years ago

Yes, feel free to transfer packages to me and I will move them over to common organization. Because they are hosted currently on personal GitHub account I think this is the easiest.

mitar commented 4 years ago

(Also add communitypackages as a maintainer to Meteor Atmosphere packages, and @meteor-community as a maintainer of any NPM package.)

vsivsi / meteor-file-collection