Skip to content
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

ember-cli #4853

Merged
merged 12 commits into from Mar 12, 2015

Conversation

Projects
None yet
5 participants
@novaugust
Copy link
Member

commented Jan 26, 2015

Closes #3237
Moves Ghost's ember admin client from the deprecated EAK to the shiny new ember-cli

Todos

  • get ember serve to show something, or at least print an error
  • move to ember-cli app layout
  • add client dependencies to ember-cli via bower / brocfile
  • move to ES6 for ember / DS
  • add sass/autoprfixer
  • asset minification
  • remove client related grunt tasks
  • update grunt to run ember-cli tasks
  • refactor client grunt tasks
  • update development/release/install workflows and readmes (no change!!? pro users will probably want to npm install -g ember-cli, but people installing don't need to)
  • remove unused root-level deps in bower.json and package.json that have moved to the client-specific versions of those files
  • discuss new code styles now that we'll be able to do es6 (http://6to5.org/docs/learn-es6/)
  • get tests working (ping @jaswilli - would love to get some help here if you feel like playing)

Not being done in this PR

  • Switch to ember-cli addons (simple-auth, etc)
  • use es6 modules from deps that export them (brocfile tinkering)

Todos from meeting 2-17

  • test release workflow (@ErisDS)
  • test different environments (dev vs prod)
  • remove client/docs

Debuggery

  • fix error page assets

@novaugust novaugust force-pushed the novaugust:ember-cli branch Feb 14, 2015

@novaugust

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2015

Massively redid this, it actually works now ;) Updated todos

@novaugust novaugust force-pushed the novaugust:ember-cli branch 2 times, most recently Feb 14, 2015

@novaugust

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2015

Updated again, first pass through the gruntfile. grunt [dev,init.prod] are all working now, but grunt dev won't auto rebuild ember yet -- need to spawn a background ember build --watch

@novaugust novaugust force-pushed the novaugust:ember-cli branch Feb 14, 2015

@jaswilli

This comment has been minimized.

Copy link
Member

commented Feb 14, 2015

Awesome. I'm traveling right now but I'll try to give this a once-over in the next couple of days.

@novaugust

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2015

Thanks!
And let's just agree not to merge any PRs that modify files in core/client
until this one's in, agreed? :D

On Sat, Feb 14, 2015 at 1:09 PM, Jason Williams notifications@github.com
wrote:

Awesome. I'm traveling right now but I'll try to give this a once-over in
the next couple of days.


Reply to this email directly or view it on GitHub
#4853 (comment).

@novaugust novaugust force-pushed the novaugust:ember-cli branch 2 times, most recently Feb 16, 2015

@jaswilli

This comment has been minimized.

Copy link
Member

commented Feb 18, 2015

I've been going over this and it's definitely functional, which is really exciting. I made some notes while going through it:

  • do we need client/tests/?
  • do we need the "qunit" dependencies in client/bower.json?
  • status of client/package.json?
    • do we need things like express?
  • remove client/app/config-dev.js and config-prod.js?
  • do we need to consider ember-cli disableAnalytics (client/.ember-cli) in regards to PRIVACY.md?
  • client lint files need to exclude client/dist/
  • what are we doing about indentation (ember-cli generates indent of 2, everything else is 4)?
  • bunch of client jscs/jshint errors that need to be dealt with
@novaugust

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2015

remove client/app/config-dev.js and config-prod.js

Yep, missed those this second time around.

what are we doing about indentation (ember-cli generates indent of 2, everything else is 4)?

I'd vote for lazily bumping up to 4 - move to 4 when someone comes in and makes a change, but leave at 2 for now

bunch of client jscs/jshint errors that need to be dealt with

Where can I see these?

package.json

@rwjblue if we never use ember serve, do we need express in the ember-cli's package.json?

disabling analytics

Yeah, i'll turn that off

do we need client/tests

I was hoping you'd be able to tell me! I figured the unit tests you've been doing would go in there, but I'm sure we can keep doing them however we currently are (..?)

q-unit deps

Probably not, if we aren't going to be using it ;)

@jaswilli

This comment has been minimized.

Copy link
Member

commented Feb 18, 2015

what are we doing about indentation (ember-cli generates indent of 2, everything else is 4)?
I'd vote for lazily bumping up to 4 - move to 4 when someone comes in and makes a change, but leave at 2 for now

The only problem with that is that JSCS is checking for an indentation of 4. So once the tests are running far enough to get to the linting, JSCS is going to go crazy over anything that was auto-generated with an indent of 2.

bunch of client jscs/jshint errors that need to be dealt with
Where can I see these?

If you fix up Gruntfile.js to exclude client/dist when building the list of files to lint, then you can run grunt jshint:client and grunt jscs:client.

do we need client/tests
I was hoping you'd be able to tell me! I figured the unit tests you've been doing would go in there, but I'm sure we can keep doing them however we currently are (..?)

I'll have to do some research on that. I just wasn't sure if you had a strategy in place that involved leaving the ember-cli generated client/tests/ in place but actually running our Ember tests from core/test/client (I realize they're not actually working right now). I'm guessing the answer is "no" and client/tests/ is just there because tests need to be sorted out still.

@novaugust

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2015

The only problem with that is that JSCS is checking for an indentation of 4. So once the tests are running far enough to get to the linting, JSCS is going to go crazy over anything that was auto-generated with an indent of 2.

Copy that, I'll just run the jshint and jscs tasks and clean up accordingly

client/tests/

Yeah, ember-cli just put them there and I wasn't sure if we wanted to work with using it as our testing framework or keep doing our own thing. Looks like that's the last big decision that needs to be made.. mind if I lean on you for it? ^ ^

@rwjblue

This comment has been minimized.

Copy link
Member

commented Feb 18, 2015

@rwjblue if we never use ember serve, do we need express in the ember-cli's package.json?

Nope, you don't need it.

Yeah, ember-cli just put them there and I wasn't sure if we wanted to work with using it as our testing framework or keep doing our own thing. Looks like that's the last big decision that needs to be made.. mind if I lean on you for it? ^ ^

In theory it would be better from ember-cli's perspective if the tests were in the "normal" location, but I think it is slightly worse ergonomics for y'all (meaning it would be nice if they could stay where they are currently). We can make fixes/changes for this in ember-cli if needed, ping me in #ember-cli and we can chat through it...

Do we need to consider ember-cli disableAnalytics (client/.ember-cli) in regards to PRIVACY.md?

Yes, that makes sense.

@jaswilli

This comment has been minimized.

Copy link
Member

commented Feb 18, 2015

In theory it would be better from ember-cli's perspective if the tests were in the "normal" location

I know we had this discussion a few months back regarding the location of Ember tests, and I was in favor of the path of least resistance then (i.e., locating them under /client), and I have to say that after working with ember-cli that I'm really in favor of doing it now.

I was able to get all the client tests running (and passing!) via ember test in like 20 minutes using ember-cli-mocha and ember-mocha. I wasted that much time just trying to guess what to change in our current customized setup (and didn't get anything close to working).

EDIT
Also, that weird problem I've been having where I can't test the rendered output of a component is gone when running the tests under ember-cli.

@rwjblue

This comment has been minimized.

Copy link
Member

commented Feb 18, 2015

I know we had this discussion a few months back regarding the location of Ember tests, and I was in favor of the path of least resistance then (i.e., locating them under /client), and I have to say that after working with ember-cli that I'm really in favor of doing it now.

Less work for me, yay!

Also, that weird problem I've been having where I can't test the rendered output of a component is gone when running the tests under ember-cli.

Awesome!

@jaswilli

This comment has been minimized.

Copy link
Member

commented Feb 19, 2015

@novaugust

I have the Ember tests working here: https://github.com/jaswilli/Ghost/tree/ember-cli-tests

I tried to create a discrete commit for every piece of work if you want to cherry-pick them onto your branch (or whatever works).

Basically what I did was:

  • Set up the ember-cli test infrastructure to use Mocha.
  • Removed all the qunit stuff.
  • Moved the Ember tests from core/test/client/unit/ into core/client/tests/unit/.
  • Removed core/test/client/.
  • Changed Brocfile.js to not run jshint.
  • Re-worked the jshint / jscs grunt tasks to accommodate the ember-cli changes.
  • Changed the indentation of ember-cli generated files from two to four spaces.
  • Made the necessary adjustments to .jshintrc files and ES6 imports to the existing Ember test files.

EDIT

The Gruntfile also needs to be adjusted to run ember test instead of the way it's currently running the Ember tests. I had that in there but it got overwritten at some point. I'll add it back in.

@jaswilli jaswilli force-pushed the novaugust:ember-cli branch Feb 19, 2015

@jaswilli

This comment has been minimized.

Copy link
Member

commented Feb 19, 2015

All the tests are now passing.

@novaugust

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2015

=O
So... squish commits (Leaving me first one + your first one with all the file moves alone) and merge after the next release?

@jaswilli

This comment has been minimized.

Copy link
Member

commented Feb 19, 2015

Yeah, definitely want to leave the move/rename files commits standalone. After that, I'd squash where it makes sense but I'd rather err on the side of more smaller commits than ending up with only a couple that are impossible to navigate.

@ErisDS

This comment has been minimized.

Copy link
Member

commented Feb 19, 2015

Definitely don't stress about commit squashing on this one. When moving/renaming files you absolutely want to keep that separate from code changes.

Where things go neatly together, squish em, otherwise don't.

@novaugust novaugust force-pushed the novaugust:ember-cli branch Feb 19, 2015

@novaugust

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2015

👍 squashed one unneeded commit from mine work
... i can't believe this is happening

Thanks a lot for taking on the tests @jaswilli!

@novaugust novaugust force-pushed the novaugust:ember-cli branch Feb 28, 2015

@novaugust

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2015

rebased on 0.5.9. @ErisDS @jaswilli @dbalders shall we take it for one last spin and get it in? :)

@novaugust novaugust force-pushed the novaugust:ember-cli branch 2 times, most recently Mar 3, 2015

@novaugust novaugust changed the title [WIP] Ember cli Ember cli Mar 4, 2015

@novaugust novaugust force-pushed the novaugust:ember-cli branch Mar 11, 2015

@novaugust novaugust changed the title Ember cli ember-cli Mar 11, 2015

@rwjblue

View changes

Gruntfile.js Outdated
@@ -31,54 +31,12 @@ var _ = require('lodash'),
return pattern.substr(1);
}
return '!' + pattern;
}).filter(function (pattern) {
// Remove empty patterns
return pattern !== '!'

This comment has been minimized.

Copy link
@rwjblue

rwjblue Mar 11, 2015

Member

Missing semicolon...

This comment has been minimized.

Copy link
@novaugust

novaugust Mar 11, 2015

Author Member

Go home Robert. You're not wanted here.

@novaugust novaugust force-pushed the novaugust:ember-cli branch Mar 11, 2015

@novaugust

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2015

Found an issue with asset delivery for non-admin assets, leaving myself a note here:
image deleted

@novaugust novaugust force-pushed the novaugust:ember-cli branch 3 times, most recently Mar 12, 2015

@ErisDS

This comment has been minimized.

Copy link
Member

commented Mar 12, 2015

@novaugust to fix that, I believe you just need to change {{asset "css/ghost.min.css" ghost="true"}} to {{asset "ghost.css" ghost="true" minifyInProduction="true"}} on L20 of user-error.hbs:

https://github.com/TryGhost/Ghost/blob/master/core/server/views/user-error.hbs#L20

@novaugust novaugust force-pushed the novaugust:ember-cli branch Mar 12, 2015

Serve files to core/built/assets/
- see core/client/lib/assets-delivery/index.js for how this is done
- Turn off ember-cli fingerprinting
- ember-cli 0.2.0; Update .npmignore
- Fallback to old version of ember-cli-sass due to lib-sass errors
- Keep ember-data at beta-14.1 until we find the dep that's breaking on snapshot.attr
- Fix release task to ignore blank lines in .npmignore

@novaugust novaugust force-pushed the novaugust:ember-cli branch to 58635b3 Mar 12, 2015

ErisDS added a commit that referenced this pull request Mar 12, 2015

@ErisDS ErisDS merged commit 5c1714a into TryGhost:master Mar 12, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@novaugust novaugust deleted the novaugust:ember-cli branch Mar 12, 2015

@letsjustfixit

This comment has been minimized.

Copy link

commented Mar 13, 2015

Congratulations!

esatterwhite added a commit to esatterwhite/Ghost that referenced this pull request Mar 27, 2015

[GST-4731] Initial setup for better configuration handling.
issue TryGhost#4731
  * install nconf
  * break fixed paths into config/overrides module
  * break defaults into config/defaults module
  * break database connection into separate module
  * rename config#load to config#read
  * move url functionality into separate module
  * remove circular depencency in errors/index.js
  * move deprication items into overrides
  * change references to config property lookups to #get / #set
  * update tests to use new configuration modules
  * allow ENV vars, command line args and multipl file locations to define configuration

[GST-4731] Move database connection into separate module.

The database connection doesn't belong in the configuration, particularly
when the connection needs the configuration before it can be created.
circular things == bad time

[GST-4731] moving things that are config defaults into defaults module

   * replace referecest to conf.foo to config.get('foo')
   * move things out of the load / init function into defaults.
     most of the values weren't being changed and were already known

[TryGhost#4731] moving remaining default configuration to defaults.js

issue TryGhost#4731
  * set sets default database configuration
  * adds GHOST_CONFIG to initial config look up paths
  * make sure paths are populated on the instance when it is exported

[TryGhost#4731] rename ConfigManager#load to read

issue TryGhost#4731
  * nconf Provider already has a load function, rename to read

[TryGhost#4731] replace config property lookups in middleware

issue TryGhost#4731
	* move config property values into defaults
	* replace config property look ups to get function calls
    * add spam rate limit values to defaults

[TryGhost#4731] remove some console.log statements

[TryGhost#4731] Update config/url.js to use config#get

issue TryGhost#4731
  * url module needs to use the get method and not try property lookups

config also seems like the wrong place for url helpers. Removing the
url methods from the configuration module eliminates a circular dependency

[TryGhost#4731] remove the url function from configuration.

issue TryGhost#4731
  * remove urlFor and urlForPost from configuration
  * move url module from config to utils
  * change references for config.urlFor|Post to url.urlFor|Post

[TryGhost#4731] Linting - No functional changes

[TryGhost#4731] Updating Unit Tests for nconf style config

use config.get / set over property lookup

[TryGhost#4731] fix deprecated items check.

issue TryGhost#4731 the function needs to check the internal state of config,
and not try property lookup

[TryGhost#4731] fix front end spec to use new config

issue TryGhost#4731
  * config must use get / set instead of property lookup

[TryGhost#4731] Check re-configration for url first.

if a url is not specified, pull the value from config store before
calculating a subdirectory

[TryGhost#4731] Correct config property look up in meta_title

issue TryGhost#4731
	* config should use get / set methods not property lookup

[TryGhost#4731] correct configuration in asset, mail, naviagtion, and footer

issue TryGhost#4731
  * change config property lookup to user #get / #set methods

[TryGhost#4731] correct configuration in asset, mail, naviagtion, and footer

issue TryGhost#4731
  * change config property lookup to user #get / #set methods

[TryGhost#4731] 100% Passing unit tests

issue TryGhost#4731
  * path.join is better at dealing with leading and trailing slashes between fragements
  * using path.join in favor of string concatenation.

[TryGhost#4731] Correcting configuration lookups in api configuration specs

issue TryGhost#4731
  * configuration lookups in api configuration integration tests use #get / #set
  * configuration lookups in api mail integration tests use #get / #set
  * configuration lookups in api themes integration tests use #get / #set

[TryGhost#4731] Linting - No functional Changes

[TryGhost#4731] Fixing database references in data utils/client

issue TryGhost#4931
  * mysql client
  * pg client
  * sqlite3 client

[TryGhost#4731] fix remaining legacy config references in test modules

issue TryGhost#4731
  * url lookup in api/users
  * paths lookup in server/apps/loader
  * everything in update-check & module spec
  * replace old database object from conf with the db connection

[TryGhost#4731] fix configuration module injection in error handling spec

issue TryGhost#4731
  * inject config instance, not a simple object
  * errors methods should use config #get / #set over property lookup

[TryGhost#4731] Use bulk insert during test runs.

posts were being inserted individually, which is really slow with
SQLite and would usually timeout, where as it would finish just fine
with postgres

[TryGhost#4731] Make sure to set GHOST_CONFIG in paths if set

[TryGhost#4731] Removing errors module from the config module.

issue TryGhost#4731 - Config module was using the errors module to gain access
to logging methods. The logging methods needed the config module creating
a circular dependency causing one of them to not load correctly.

Logging should not be in an errors module. There needs to be a module
that is not dependant on something else with in ghost. Configuration being
the one thing that most everything else needs should be that module

[GST-4731] moving things that are config defaults into defaults module

   * replace referecest to conf.foo to config.get('foo')
   * move things out of the load / init function into defaults.
     most of the values weren't being changed and were already known

[TryGhost#4731] remove some console.log statements

[TryGhost#4731] Linting - No functional changes

[TryGhost#4731] fix front end spec to use new config

issue TryGhost#4731
  * config must use get / set instead of property lookup

[TryGhost#4731] correct configuration in asset, mail, naviagtion, and footer

issue TryGhost#4731
  * change config property lookup to user #get / #set methods

[TryGhost#4731] correct configuration in asset, mail, naviagtion, and footer

issue TryGhost#4731
  * change config property lookup to user #get / #set methods

[TryGhost#4731] Linting - No functional Changes

[TryGhost#4731] fix remaining legacy config references in test modules

issue TryGhost#4731
  * url lookup in api/users
  * paths lookup in server/apps/loader
  * everything in update-check & module spec
  * replace old database object from conf with the db connection

[TryGhost#4731] fix configuration module injection in error handling spec

issue TryGhost#4731
  * inject config instance, not a simple object
  * errors methods should use config #get / #set over property lookup

[TryGhost#4731] Use bulk insert during test runs.

posts were being inserted individually, which is really slow with
SQLite and would usually timeout, where as it would finish just fine
with postgres

[TryGhost#4731] Removing console statements in errors on fork utils.

noissue - no longer needed

[TryGhost#4731] linting - no functional changes.

no functional changes

[TryGhost#4731, TryGhost#4853] including clientAssets in default configuration

issues TryGhost#4853, TryGhost#4731
build scripts path was removed and client assets was used in its place

[TryGhost#4731] - Documentation for config modules

no functional changes

[TryGhost#4731] linting - no fnctional changes

[TryGhost#4731] Fix config spec to work with new config

[TryGhost#4731] Correct config spec loading of defaults.

The config.example is really no longer relevant and we don't care so much.
and values can be merged. we really care about the connection specific
values in this situation

[TryGhost#4731] eliminate the need to compute inside of configuration.

recalculating paths everytime something changes is a bit messy,
especially when the destination folder is always the same. Just
run path.join when you need it

[TryGhost#4731] resolve config file paths.

allows for reletive or absolute locations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.