-
Notifications
You must be signed in to change notification settings - Fork 49
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
Simplified exports in blueprints #137
Conversation
I didn't know there were node tests in here as well, I'll get those fixed. |
2aebcda
to
f51b247
Compare
@kimroen pushed updated code with amended tests. There is only one failure (beta) in Travis right now that looks like it needs poking to be rerun. |
@kimroen, All builds are green now, do you think it should be merged, or are you still missing something? |
@@ -1,4 +1,2 @@ | |||
<%= camelizedModuleName %> = () -> | |||
export default () -> |
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.
In the original blueprint this is a named function, but I haven't found a way to export a named function with the CoffeeScript syntax, so I'm not sure if it can be done.
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.
@kimroen I think you can do that with
export <%= camelizedModuleName %> () ->
Like we have done here: https://github.com/kimroen/ember-cli-coffeescript/pull/137/files#diff-f9c146914b83a86c6a8d484dc1d877d6R4
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.
I'll do some testing and create a PR for that.
This looks good - thanks again! 🎉 |
I noticed now that this missed some files, like the initializers. |
@kimroen I'll check those out later today/tonight |
This PR changes the blueprints from using a named variable to doing direct exports of the code. In addition to being more easy on the eyes, it fixes a problem with the compiled javascript file containing an undeclared variable in newer versions of the CoffeeScript compiler.
Fixes #135
Fixes #134
I have tested to create all of the blueprints changed:
And the tests pass with the exception of
view
which I assume is due to the fact that a fairly recent Ember version is used which no longer supportsview
s. I guess this blueprint can be removed at some point unless we want to keep it for people using it with older Ember version. On the other hand a case can be made for not helping them to create code that is deprecated in newer versions of Ember.