meteor-file-collection icon indicating copy to clipboard operation
meteor-file-collection copied to clipboard

Upgrade to v2 of coffee

Open brucejo75 opened this issue 7 years ago • 30 comments

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

Changes to package.js

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.

brucejo75 avatar Feb 16 '18 15:02 brucejo75

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.

vsivsi avatar Feb 16 '18 21:02 vsivsi

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 avatar Feb 16 '18 23:02 brucejo75

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.

brucejo75 avatar Feb 16 '18 23:02 brucejo75

@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.

vsivsi avatar Feb 17 '18 00:02 vsivsi

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

brucejo75 avatar Mar 02 '18 20:03 brucejo75

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 avatar Mar 06 '18 03:03 vsivsi

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

vsivsi avatar Mar 06 '18 03:03 vsivsi

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.

edemaine avatar Mar 06 '18 13:03 edemaine

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

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

brucejo75 avatar Mar 06 '18 20:03 brucejo75

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 avatar Mar 06 '18 20:03 vsivsi

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

Package.onUse(function(api) {
  api.use('[email protected]_1', ['server','client']);
  api.use('[email protected]', 'server');
  api.use('[email protected]', ['server', 'client']);
  api.use('[email protected]', 'server');
  api.use('[email protected]', ['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('[email protected]_1', ['server', 'client']);
  api.use('[email protected]', ['server', 'client']);
  api.use('[email protected]', ['server','client']);
  api.use('[email protected]', ['server','client']);
  api.use('[email protected]',['server','client']);
  api.use('[email protected]', ['server', 'client']);
  api.use('[email protected]', ['server', 'client']);
  api.use('[email protected]', 'client');
  api.addFiles('test/file_collection_tests.coffee', ['server', 'client']);
});

vsivsi avatar Mar 06 '18 20:03 vsivsi

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.

vsivsi avatar Mar 07 '18 00:03 vsivsi

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.

brucejo75 avatar Mar 07 '18 13:03 brucejo75

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.

edemaine avatar Mar 07 '18 13:03 edemaine

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.

vsivsi avatar Mar 07 '18 17:03 vsivsi

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

apendua avatar Mar 26 '18 10:03 apendua

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

antoninadert avatar Mar 27 '18 14:03 antoninadert

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

brucejo75 avatar Mar 27 '18 21:03 brucejo75

Any chance of merging this soon?

hluz avatar May 08 '18 09:05 hluz

Also looking forward to see this merged!

juliomac avatar Jun 20 '18 20:06 juliomac

When merge?

thumptech avatar Sep 24 '18 01:09 thumptech

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?

antoninadert avatar Sep 24 '18 10:09 antoninadert

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.

edemaine avatar Dec 16 '18 16:12 edemaine

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.

brucejo75 avatar Dec 16 '18 18:12 brucejo75

@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.

edemaine avatar Dec 17 '18 15:12 edemaine

@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

vsivsi avatar Oct 08 '19 20:10 vsivsi

@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/?

edemaine avatar Oct 08 '19 21:10 edemaine

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.

vsivsi avatar Oct 08 '19 21:10 vsivsi

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 avatar Oct 08 '19 21:10 mitar

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

mitar avatar Oct 08 '19 21:10 mitar