-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Convert to Ember CLI addon. #9
Conversation
EmberCLIEmberOrbit.prototype.included = function included(app) { | ||
this.app = app; | ||
|
||
var imports = app.options.getEnvJSON(this.app.env).ORBIT.IMPORTS; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should have some sort of check here to make sure ORBIT.IMPORTS
exists and throw a usable error to the user when it doesn't.
Also, It should always have orbit.js and orbit-common.js, they can't opt to not include those since ember-orbit won't work without them.
It could be simplified to something like:
orbit: {
jsonapi: true/false,
localStorage: true/false
}
The user won't know the file names since they are all container within the ember-orbit directory.
@MatthewHager thanks for the PR and thanks to @jakecraige for the review. This commit is a great start, and I'm now working with it locally and tweaking it mostly in line with Jake's points. I'll try to get orbit and ember-orbit published on npm within the next day. |
After reading this comment and talking with @stefanpenner, I'm afraid that I need to backtrack on my goal of making ember-orbit an Ember CLI addon. Stef confirmed that addons aren't meant for browser libs but rather for extending the tool. A good example would be an ember-orbit generator lib, which could be built as a separate addon and published via npm. Thanks again for this PR - the work is appreciated and may yet be useful in the future. The addon story and ember cli in general are still in flux and moving quickly. For now, I'm going to set up a separate ember-orbit bower repo and improve the instructions for getting up and running ASAP. |
Cool, thanks for your work and sharing! 👍 |
It is inconsistent with https://www.npmjs.org/package/ember-cli-ember-data, and a lot of packages here: https://www.npmjs.org/browse/keyword/ember-addon I suppose once you have it on bower I will just make a wrapper for it and call it ember-cli-bower-ember-orbit, hah |
@MatthewHager I hear you - the extra import boilerplate is a hassle which is why most of those packages exist. I'm relatively new to ember-cli myself, which is why I confirmed this with @stefanpenner. Stef's comment mentions a strategy to remove this boilerplate:
I'd prefer to see how this plays out before creating an addon, and it doesn't sound like we'll need to wait long. I'll reopen this PR so we can track this goal in this repo. |
I agree ember-cli addons should not be used as yet another package manager. |
Addons use existing package managers. They provide the convenience of handling the boilerplate configuration so that once you add the package (via npm), you are all set. I'm not even sure why this has been a debate. I added code which allows you to run a single command and be up and running with orbit and ember-orbit properly installed, configured and ready to go. Many other packages including core packages are doing this exact same thing. I pulled the code for doing this from ember-cli-ember-data. That is a browser package no different than this one. I understand in the future ember-cli might come out with a different solution but there is no telling how far off that is. When that happens the existing packages will be converted to use the new format (most likely including ember-cli-ember-data). We have this for right now as the ONLY way to add packages in a convenient zero config way. |
@MatthewHager Minimizing config / boilerplate is a goal of mine too and we'll make it happen soon one way or another. I'm tracking ember-cli closely to see where this goes and will keep this PR open until a clear direction emerges. |
I just made it happen and in the way that everyone is doing it right now but its all good. It was fun coding it either way and I've got my fork. |
One more thing to note, here is a project doing something similar that we actually are using for now instead: https://github.com/kurko/ember-sync. It's an addon and it is very easy to use. No one argued about it being an addon. Everyone is just happy that it is easy to install and use. |
@MatthewHager, I believe @dgeb has his reasons for taking these kind of decisions. As he said, he seems to be on the same boat: this library it should be easy to install and use without boilerplate code. But, the Ember ecosystem is quite young and unstable still, and some people think it makes sense to take more time to make any decision, and be quite certain that it will be a step in the right direction. I myself find that reliable in a very valuable way (albeit not so "agile"). I spent the whole day yesterday trying to run ember-sync. There's many bugs, relationships don't work, and more worrying is the fact that current maintainer seems to have many unknown-unknowns. I don't blame that on him, I actually appreciate that he would release his code early for everyone to use and build upon. Taking decisions faster or slower both come with advantages and drawbacks. I don't think we need to get rude on anyone's philosophy or motivations, and I think discussions should be tolerant of disagreements. And thanks for your code! |
I apologize if I came off rude. I was only trying to provide evidence to the contrary of the decision that was made. Just trying to push the project forward and make it easier for everyone to use. Most of the other projects we are using are in the form of addons and it has been awesome. I'll leave it alone now. |
No worries here, guys. We're working toward the same goals, and we'll get there one way or another very soon :) I'm simply waiting for a little clarity on the ember-cli situation before acting. |
@dgeb Just curious what your thoughts on this are now. The pattern is now more defined that it was before. The way it works now is that the addon, in this case, ember-orbit. Would provide a generator called obviously this PR would need to be updated and I can help @MatthewHager to do that if it's something you support doing. |
how does this differ from?
app.import('bower_components/orbit/orbit.js'); Maybe this is adding more then I realized, like components/views/helpers/blueprints etc., and then it should be pulled in as an add-on. But simply to replace 2 lines of code, seems like a whole lot of ceremony. |
@stefanpenner Check out the readme ember-cli install steps There's a lot more than just that to get rolling with this. |
so // Required Orbit imports
app.import('bower_components/orbit.js/orbit.amd.js', {
exports: {'orbit': ['default']}
});
app.import('bower_components/orbit.js/orbit-common.amd.js', {
exports: {'orbit-common': ['default']}
});
// Optional import of local storage source
app.import('bower_components/orbit.js/orbit-common-local-storage.amd.js', {
exports: {'orbit-common/local-storage-source': ['default']}
});
// Optional import of JSON API source and serializer
app.import('bower_components/orbit.js/orbit-common-jsonapi.amd.js', {
exports: {'orbit-common/jsonapi-source': ['default'],
'orbit-common/jsonapi-serializer': ['default']}
});
// Required Ember-Orbit import
app.import('bower_components/ember-orbit/ember-orbit.amd.js', {
exports: {'ember-orbit': ['default']}
}); will soon (via http://github.com/abuiles/broccoli-petal) turn into, which seems nice and concise. // Required Orbit imports
app.import('bower_components/orbit.js/orbit.amd.js');
app.import('bower_components/orbit.js/orbit-common.amd.js');
// Optional import of local storage source
app.import('bower_components/orbit.js/orbit-common-local-storage.amd.js');
// Optional import of JSON API source and serializer
app.import('bower_components/orbit.js/orbit-common-jsonapi.amd.js');
// Required Ember-Orbit import
app.import('bower_components/ember-orbit/ember-orbit.amd.js'); and I can see that further improvements, to simply drive the import Orbit from 'bower:orbit' which would be basically 0 need for an addon. The last step may still be a ways away, and we may discover it's actually bad for some reason. I would suggest not to create a wrapper, let it be verbose for now. |
@stefanpenner Thanks for weighing in. I agree that we don't want addons that simply wrap bower imports. This seems especially true in a lib such as orbit, in which the libs to import will vary based on a project's needs. Plus I'm encouraged by the simplified syntax that will be available with broccoli-petal - it's nice and clean.
I would like to start providing blueprints for models, which seems to tip the balance toward making ember-orbit an addon after all. This also seems consistent with ember-data's approach. @jakecraige Thanks for your offer to help @MatthewHager update this PR. That would be much appreciated! Note: I'd like to avoid environment variables that simply configure optional imports. It seems more straightforward to just include extra instructions in the README for importing the local storage / json api sources. This will make configuration of these optional sources consistent with other optional sources not contained in the main orbit repo (e.g. a socket source). I suppose the other approach we could take is to configure all the optional imports from the main repo, and let devs comment out the ones they don't want. Hmmm... but maybe that would get annoying with the ember-cli upgrade process rewriting those changes? |
@jakecraige @stefanpenner @rwjblue As I said above, I'm pretty sure that I want an ember-orbit addon for ember-cli to support generators / blueprints. However, I'm still a little flummoxed with the Best Practices (TM) here. My question is whether this addon should be maintained in a completely separate repo instead of in ember-orbit itself. It will obviously be published separately (npm vs. bower) and may not need to stay in lock-step versioning with ember-orbit. I imagine ember-orbit itself will be updated much more frequently than its blueprints and other addon capabilities. And yet I see that ember-data's addon is maintained in the main ember-data repo. I know some of these conventions are being worked out, but do you have a recommendation here? |
I say, lets build an addon. The pattern is much more stable now and model blueprints would be nice. @dgeb Have you seen ember-simple-auth? All the packages/libraries are maintained in a single repository and separate repositories are used to maintain the different addons for each library. core:
addons:
Seems like that setup has worked pretty well. |
Superceded by #87 |
Fixes #8. I've converted this into an Ember CLI addon. I've made it to where you can specify which orbit files you want included in your environment.js file. I've updated the README but it the install instructions won't fully work until my pull request is accepted. Further, the instructions will change when it goes on npm.