mizzao / meteor-sharejs

Meteor smart package for transparently adding ShareJS editors to an app
MIT License
225 stars 53 forks source link

Use npm dependencies and module system #75

Closed DavidSichau closed 8 years ago

DavidSichau commented 8 years ago

Sharejs now imports all dependencies as npm dependencies. This avoids the setup of gitsubmodules.

It uses the module system to simplify the handling of the dependencies.

Most of the code was rewritten in plain js with ecmascript.

Discussion:

The following points are no solved to my satisfaction:

mizzao commented 8 years ago

This looks fantastic!

One way to get around the dependency on the external CDN that you outlined is to use addAssets to serve files from the npm module, see

https://github.com/mizzao/meteor-sharejs/blob/749f8135365bb61420b4edd2d17b33f8a5e10eee/sharejs-base/package.js#L23

but you may want to do it programmatically as in

https://github.com/mizzao/meteor-sharejs/blob/749f8135365bb61420b4edd2d17b33f8a5e10eee/sharejs-ace/package.js#L69)

then set this as the basePath on the client. It's going to look very funky, probably packages/mizzao:sharejs/.npm/package/node_modules/ace/... but it'll work.

You get rid of the dependency on the external CDN, but you do have a somewhat brittle implementation (if Meteor changes that directory structure). However, it's worked since 0.8 or whenever I started this, so I think it will be fine.

Awesome that you rewrote in ES6 instead of coffee, and I agree that dropping the auth system is probably fine until you have time to implement a better one. #6 was pretty random anyway.

Let me know when you get this merged; that would probably be a good time for me to put the demo back up on Galaxy.

DavidSichau commented 8 years ago

So we had this code running in our app since 1 month and it seems to be fine.

So I would suggest to merge this pull request even if I still did not found a way to include ace in a clean way.

However we have a breaking change that we now drop the authentication.

mizzao commented 8 years ago

Yeah, feel free to remove the authentication and remove the documentation for it!

Great to hear that you have this working in production. I am very happy to have someone else using and maintaining it!

Did I give you publish permission on Atmosphere for this package?

DavidSichau commented 8 years ago

Yes I have the rights. I will publish it tomorrow afternoon (European).

This is one of our core packages we use, thanks a lot for your efforts to provide this package.