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

Use ember-suave and standardise client's es6 usage #6020

Merged
merged 1 commit into from
Nov 30, 2015

Conversation

kevinansfield
Copy link
Member

no issue

  • adds ember-suave dependency to enforce es6 styles
  • update files to match Dockyard's Javascript (with slight modifications) and Ember style guides

ES6 is seeing rapid adoption across the ember core and community in general, this is an attempt to bring Ghost in line with the accepted coding style.

Aside from the general es6-isms, Ember.* and DS.* modules are pulled out and defined at the top of the files ready for when these modules are pulled out into ES2015 modules that can be imported directly. I believe there are plans to eventually use this within the build process to only pull in Ember code that our application is using giving us smaller builds and faster download/parsing.

I've created as a quick spike for now in order to gather thoughts and discussion from the rest of the team. Please let me know if there's anything in particular you think is confusing, difficult to parse or too different to the server-side!

Outstanding issues:

  • Ember.Object, Ember.String, Ember.$ have not yet been moved into the Ember object destructuring assignment as they generate warnings about overriding existing vars. Need to decide to skip those for now or find a way to suppress the warnings.
  • Ember.A() (builds an Ember.NativeArray instance) and Ember.K() (empty function that returns this) have not been moved to the Ember destructuring assignment because I'm not a fan of the one-char function names.
  • Have we gone far enough with the fat arrow functions? Currently all methods inside a property method use fat arrows to preserve the this scope, should the fat arrow syntax be used anywhere else? Is the non-parens version preferable in certain circumstances?

@kevinansfield kevinansfield changed the title Spike: Use ember-suave and es6-ify client files Spike: Use ember-suave and standardise es6 usage in the client Oct 28, 2015
@kevinansfield kevinansfield changed the title Spike: Use ember-suave and standardise es6 usage in the client Spike: Use ember-suave and standardise client's es6 usage Oct 28, 2015
@kevinansfield kevinansfield added good first issue [triage] Start here if you've never contributed before. affects:admin Anything relating to Ghost Admin and removed good first issue [triage] Start here if you've never contributed before. affects:admin Anything relating to Ghost Admin labels Oct 30, 2015
@kevinansfield kevinansfield changed the title Spike: Use ember-suave and standardise client's es6 usage WIP: Use ember-suave and standardise client's es6 usage Nov 2, 2015
@kevinansfield kevinansfield changed the title WIP: Use ember-suave and standardise client's es6 usage [WIP] Use ember-suave and standardise client's es6 usage Nov 2, 2015
@kevinansfield
Copy link
Member Author

Regarding const / let usage, there's been some interesting discussion where the outcome has mostly been "only use const where you would normally use var FOO = 'bar'; at the module-level".

DavyJonesLocker/ember-suave#77
emberjs/ember.js#11874 (comment)

const to express variable bindings that should ABSOLUTELY not change, not bindings that happen to not change
let express variable bindings that may or may not change.

The goal of this difference as señor @wycats explain to me, is to not overly abuse const, doing so will likely desensify devs and diminish its value.

I think we should probably follow suit as I can't fault their arguments and it will probably solve a lot of explanation about const being constant-reference not constant-value in JS-land.

@kevinansfield
Copy link
Member Author

Regarding Ember.K and Ember.A usage in Ember's source code...

Ember.K is now defined as a top-level function in any file that uses it, e.g. https://github.com/emberjs/ember.js/blob/master/packages/ember-views/lib/mixins/visibility_support.js#L12. Our use of Ember.K is pretty minimal so I'm happy to follow suit as it provides more visibility to what K actually does.

Ember.A() is not strictly needed as we haven't disabled prototype extensions but I think it makes sense to standardise on using it in case we want to pull code out into addons or disable prototype extensions in the future. Ember now imports A as emberA (https://github.com/emberjs/ember.js/blob/master/packages/ember-runtime/lib/computed/reduce_computed_macros.js#L13) which again makes sense to me.

When I get around to rebasing this I'll be making the above changes too unless there are any objections.

@rwjblue
Copy link
Contributor

rwjblue commented Nov 17, 2015

I may be biased about ember-suave, but I do like it. 😜

@ErisDS
Copy link
Member

ErisDS commented Nov 17, 2015

Regarding const / let usage

This makes perfect sense IMO. I'm keen to use let by default - kind of looking forward to when we can do that on the server side too.

@kevinansfield kevinansfield changed the title [WIP] Use ember-suave and standardise client's es6 usage [HOLD] Use ember-suave and standardise client's es6 usage Nov 25, 2015
@kevinansfield kevinansfield changed the title [HOLD] Use ember-suave and standardise client's es6 usage [Hold] Use ember-suave and standardise client's es6 usage Nov 25, 2015
@kevinansfield kevinansfield changed the title [Hold] Use ember-suave and standardise client's es6 usage Use ember-suave and standardise client's es6 usage Nov 30, 2015
no issue
- add ember-suave dependency
- upgrade grunt-jscs dependency
- add a new .jscsrc for the client's tests directory that extends from client's base .jscsrc
- separate client tests in Gruntfile jscs task so they pick up the test's .jscsrc
- standardize es6 usage across client
sebgie added a commit that referenced this pull request Nov 30, 2015
Use ember-suave and standardise client's es6 usage
@sebgie sebgie merged commit e5d9865 into TryGhost:master Nov 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants