feat(format): add code formatting #200
Conversation
filipesilva
force-pushed
the
ng-format-command
branch
3 times, most recently
from
b437e74 to
bd7edef
Compare
|
LGTM. |
filipesilva
force-pushed
the
ng-format-command
branch
from
bd7edef to
1f89a2b
Compare
|
|
||
| var Promise = require('ember-cli/lib/ext/promise'); | ||
| var Task = require('ember-cli/lib/models/task'); | ||
| var exec = Promise.denodeify(require('child_process').exec); |
There was a problem hiding this comment.
OOC, couldn't Promise and exec be imported when the task is run (so that addon/ng2/index.js "loads" faster) ?
(I guess they are already imported elsewhere, so it won't make any real difference, but wouldn't it theoretically be better ?)
There was a problem hiding this comment.
I suppose, in the case where they are first imported here, it would be faster, yes.
Leaving them at the top seems to be the normal usage pattern, and it's also a bit more forwards compatible with the ES6 mindset. I'm also not a big fan of conditional imports myself, they're sometimes hard to find, test and debug.
Come to think of it, I remember reading they opted with toplevel imports for a number of reasons that were meant to get around problems with node's modules system.
I don't see much advantage in this case to only requiring them when the task is run.
There was a problem hiding this comment.
Yeah, it was more of a theoretical question, because they are surely already imported elsewhere.
Thx for the response.
I'm also not a big fan of conditional imports myself
Interesting (considering how chalk is imported below)... 😛
There was a problem hiding this comment.
Ah yes - guilty as charged. I copy pasted an existing task and took it down to it's bare minimum. The chalk require was there as is, and me not being fully knowledgeable of ember-cli, simply left it.
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Close #143