Ghost: ES6 migrations

[vikaspotluri123]

This is something ~I plan on working on in the middle of May~ a bunch of people are helping out on! 😄

• var -> let / const
• _.includes(array) -> array.includes (dropping v4 support at the end of April makes this possible)
• _.each(array) -> array.forEach
• _.isArray(array) -> Array.isArray(array) - Only if it removes the necessity of requiring lodash in a specific file
• _.isEmpty(array) -> array.length === 0 (or !array.length)
• Remove other lodash dependencies as needed on a file-specific basis (the goal isn't to purge lodash, it's to reduce unnecessary requires of the large library)
  
  
  
  • In that note, use Object destructuring to import required components as needed
  
  
  • Use you might not need lodash but don't go overboard
  
  
• remove usage of var self = this (arrow functions instead)
• Use defaults (I'm not sure if this is applicable) (i.e. (a = 2, b = 2) => a + b)
• Use template literals when possible

---

Checklist:

• [ ] Arrow functions do not replace named functions
• [ ] var -> const / let
• [ ] Array.(includes / each / isEmpty / isArray) lodash usage minimized
• [ ] Template literals
• [ ] Import specific lodash modules instead of entire library

[karaggeorge]

Hello, I'd like to try this. I'm really comfortable with es6 syntax and I think I could use this to get more familiar with the code base.

Could you please help me with what you are looking for exactly?

• Should the entire project be migrated? Or just a specific part (server?).
• Is babel used? or should only syntax that is available in the current Node version be migrated?

[vikaspotluri123]

Hey @karaggeorge 👋 I was planning on migrating the entire Ghost repo (so specifically TryGhost/Ghost) after I get done with my semester, but I'm definitely not opposed to getting help 😉

@kirrg001 how would you suggest multiple people working on the same thing? I was thinking @karaggeorge PRs into my repo and enables allow edits from maintainers so I can push commits as well 🤔

Babel won't be used since that will add additional dependencies; the plan is to merge the migrations after v4 is deprecated (April 30)

[kirrg001]

Thanks for interest and help 😎

Multiple people is fine, but please only very small pull requests - 1-2 files per PR. Otherwise this can heavily conflict with existing pull requests and takes too much time to review. It also means, if multiple people would like to help, this needs communication here, who takes over which folder. And please do not rewrite logic, only ES6 style changes 👍

If you want to rewrite logic, please argue in this issue and wait for confirmation. After confirmation, you can submit this rewrite as separate PR.

e.g. changing _.includes(array) -> array.includes is absolutely fine and does not need confirmation

e.g. rewriting a whole function needs confirmation

---

Please do after we've dropped Node v4 🙌🏻

Should the entire project be migrated? Or just a specific part (server?).

Only the server and test folder.

Is babel used?

No

[karaggeorge]

@vikaspotluri123 @kirrg001

Please do after we've dropped Node v4 🙌🏻

Do you mean, start working on it after Node v4 is deprecated? Or it's ok to create PRs now, but they won't get merged until it's deprecated?

Sounds good. I'm planning on working on it now (in the next few days) so it shouldn't cause conflicts. I'll keep every PR small 1-2 files.

[vikaspotluri123]

Sounds good. I'm planning on working on it now (in the next few days) so it shouldn't cause conflicts. I'll keep every PR small 1-2 files.

This sounds good - can you let me know what folder you plan on migrating first? I know I probably won't have time right now to work on it, but if I somehow find it, I don't want to be stepping on your toes 😃

[karaggeorge]

I'll start with core/server/api/ 😄

[kirrg001]

Do you mean, start working on it after Node v4 is deprecated?

After Ghost has dropped it.

[vikaspotluri123]

Checklist:

If anyone knows of a way to make it less obtrusive, that would be great 😏

• [ ] helpers (WIP @hkd987, #9662, #9663, #9669)
• [x] api (@tryghost ~@mandeepm91~, #9733, #9756, #9761, #9876)
• [ ] apps (WIP @tiendq, #9667, #9707)
  
  
  
  • [ ] /amp
  
  
  • [ ] /amp/lib/helpers
  
  
  • [ ] /amp/lib
  
  
  • [ ] /amp/lib/views
  
  
  • [ ] /private-blogging
  
  
  • [ ] /private-blogging/lib/helpers
  
  
  • [ ] /private-blogging/lib
  
  
  • [ ] /private-blogging/lib/views
  
  
  • [ ] /subscribers (#9688)
  
  
• [ ] adapters (@billfienberg, #9689, #9696 #9698, #9700)
  
  
  
  • [ ] /scheduling
  
  
  • [ ] /scheduling/post-scheduling
  
  
  • [ ] /storage
  
  
  • [ ] config
  
  
  • [ ] /env
  
  
  • [ ] web (WIP @tryghost ~@larsnolden~, #9729, #9737)
  
  
  • [ ] /admin
  
  
  • [ ] /admin/views
  
  
  • [ ] /api
  
  
  • [ ] /middleware
  
  
  • [ ] /middleware/api
  
  
  • [ ] /middleware/validation
  
  
  • [ ] /site
  
  
  • [ ] lib (WIP @iancottam91, #9779)
  
  
  • [ ] /common (#9779)
  
  
  • [ ] /fs
  
  
  • [ ] /fs/package-json
  
  
  • [ ] /image
  
  
  • [ ] /mobiledoc/atoms
  
  
  • [ ] /mobiledoc/cards
  
  
  • [ ] /mobiledoc/converters
  
  
  • [ ] /mobiledoc
  
  
  • [ ] /promise
  
  
  • [ ] /security
  
  
  • [ ] /social
  
  
  • [ ] data
  
  
  • [ ] /db
  
  
  • [ ] /export
  
  
  • [ ] /importer/handlers
  
  
  • [ ] /importer/importers/data
  
  
  • [ ] /importer/importers
  
  
  • [ ] /importer
  
  
  • [ ] /meta
  
  
  • [ ] /migrations/hooks/init
  
  
  • [ ] /migrations/hooks/migrate
  
  
  • [ ] /migrations/init
  
  
  • [ ] /migrations/versions/*/
  
  
  • [ ] /schema
  
  
  • [ ] /schema/clients
  
  
  • [ ] /schema/fixtures
  
  
  • [ ] /validation
  
  
  • [ ] /xml/sitemap
  
  
• [ ] models
  
  
  
  • [ ] /base
  
  
  • [ ] /plugins
  
  
  • [ ] /relations
  
  
  • [ ] public
  
  
  • [ ] services
  
  
  • [ ] /apps
  
  
  • [ ] /auth
  
  
  • [ ] /channels
  
  
  • [ ] /mail
  
  
  • [ ] /mail/templates
  
  
  • [ ] /mail/templates/raw
  
  
  • [ ] /permissions
  
  
  • [ ] /route
  
  
  • [ ] /rss
  
  
  • [ ] /settings
  
  
  • [ ] /themes
  
  
  • [ ] /themes/config
  
  
  • [ ] /url
  
  
  • [ ] translations
  
  
  • [ ] views
  
  
  • [ ] Tests
  
  
  • [ ] Root files
  
  

[karaggeorge]

@vikaspotluri123

I guess I'll wait until Ghost drops it before working on this 😄

[kirrg001]

Yeah please wait 2 weeks minimum, because of the Node v4 drop and we will merge dynamic routing in a 1-2 weeks, which moves some files around. It's quite a big feature. I'll give the go here if you want :)

[karaggeorge]

@kirrg001 sounds good!

[kirrg001]

FYI: We also have a couple of dependencies, which would need an ES6 update. I am not sure how much ES6 updates are required, but worth double checking.

e.g.

• gscan
• bookshelf-relations
• knex-migrator
• GQL
• Ignition

Let me know if you are interested in helping out there as well 🤠

---

Regarding the ES6 updates in Ghost. Dynamic routing was not merged yet and i would love to wait with the ES6 updates till then. As said, i'll let you know, when you can start with ES6 PR's for Ghost.

[tiendq]

I'd like to try this too :). It's likely done file by file but how can we prevent same modification from happening? Can we break it down to many small issues each have its own PR later? Once an item is picked from @vikaspotluri123 list, for example, it'll be crossed out and added an issue number so others can simply ignore it.

[vikaspotluri123]

@Tiendq you're on the right track wrt the plan - whenever someone starts working on something, they will post it as a comment here, and I will update the master list w/ WIP by {username}. The person will create PR(s) which update files to support es6; the number of PRs per item is variable based on file length and file complexity. Finally, when all of the files are updated / migrated, I will update the master list by removing the WIP label and checking it.

As far as I know, maintainers can also edit my list so if I don't get around to it, someone hopefully will.

Right now, as @kirrg001 we're holding off on starting since the Ghost Team wants to merge dynamic routing first.

[billfienberg]

I'd like to help. I realize we have to wait until the dynamic routing is merged in, but if there's anything I can do before then, please let me know.

[vivekannan]

I would like to be involved in this as well.

[mikkqu]

I'd be glad to work on this too. Subscribing to a thread to not miss the updates.

[kirrg001]

Sorry. I was three weeks off. As soon as https://github.com/TryGhost/Ghost/pull/9596 get's merged, you can start with the ES6 migrations. Thanks :)

[hkd987]

currently working on this as well.
Have a pull request in on ./helpers/asset.js
Will start working on the smaller files in this folder when I have time.

https://github.com/TryGhost/Ghost/pull/9662

[vikaspotluri123]

@hkd987 I've updated the checklist and marked you as working on helpers/*

[tiendq]

If no one picked it, I could start with controllers or apps tomorrow :)

[vikaspotluri123]

@tiendq starting is still on hold for now; see #9662 (comment)

However, I've marked you as working on the controllers folder

[kirrg001]

https://github.com/TryGhost/Ghost/pull/9596 was merged 🚓

[tiendq]

@vikaspotluri123 controllers has been disappeared :D, so I'd like to give another try with apps :)

[mandeepm91]

Hi @vikaspotluri123 I would like to help with this. Let me know which directory I can pick up.
Thanks!

[vikaspotluri123]

@mandeepm91 Feel free to pick any folder that isn't checked off and does not have WIP to the right of it!

[kirrg001]

Before you raise a PR, ensure you have read this conversation about arrow functions. Feel free to add your opinion.

[kevinansfield]

FYI, for anyone working on this there are numerous codemods available to automate a lot of the es6 transformation work. This article is a nice introduction to codemods https://medium.com/airbnb-engineering/turbocharged-javascript-refactoring-with-codemods-b0cae8b326b9

[mandeepm91]

@vikaspotluri123 I'll start with the api folder

[tiendq]

@kirrg001 @kevinansfield I see many named callback functions, and function names are not used anywhere. Obviously I will replace them with arrow functions but I'm not sure if it's right decision :)

[mandeepm91]

@vikaspotluri123 it's my first contribution so if I am making any mistakes in following the process, please let me know.
Thanks

[mandeepm91]

@vikaspotluri123 @kirrg001 @kevinansfield

The linter is showing some issues which aren't really issues in my opinion. For example, consider the code:

  .then(() => _private.readRedirectsFile());

It raises the issue that _private.readRedirectsFile() should be inside a block. So, consequently I am changing this to:

  .then(() => { return _private.readRedirectsFile(); });

But I think the former approach is more concise and more readable. What do you think ?

[mandeepm91]

@kirrg001 Sorry I missed your comment regarding including small PRs with only 1 or 2 files in a PR. I was earlier planning on converting the entire core/server/api directory and then raising a PR. I have already converted 12 files so far. Will it be ok if I raise a PR for those 12 files?

[tiendq]

@mandeepm91 Agree, linter is strict more than necessary in certain situations. I have also noticed another case: .then((result) => ...), parentheses are must while .then(result => ...) is fine.

[vikaspotluri123]

wrt linting... @kirrg001 is there any plan to use the Ghost eslint plugin for this repo? If so, we might be able to add another npm / grunt script (i.e lint:bleeding) which uses an eslintrc file based on the plugin. Then, for PRs related to ES6, there would be an additional requirement that yarn lint:bleeding passes which would allow for the gradual migration to the new rules 🤔

[Huc91]

I also would like to help you out, can I still refer to the checklist posted by @vikaspotluri123 as a reference to what still need to be done?

[vikaspotluri123]

Yep, @Huc91! I think I've notated all the PRs, but the assignments are up to date. @ everyone else - if a PR doesn't get added to the list after a day or two feel free to ask me to add it 😄

[vikaspotluri123]

As mentioned in #9689 (comment), @billfienberg is working on the adapters folder

[Huc91]

@vikaspotluri123 Thanks, I'm gonna take a look

[larsnolden]

@vikaspotluri123 I am working on everything inside server/web. More to come

Questions:

web/middleware/static-theme.js
=> do we want to get rid of _.inlcudes?

web/middleware/url-redirects.js
=> Line8: Can we use Object destructuring?
=> Line25: Can we use JS shorthand property names? (ES5)

These cases are not part of the initial es6 migration 'guide', so I want to confirm before making these changes.

[vikaspotluri123]

@larsnolden I've marked you as working on web 👍

All 3 of your questions have been discussed in other issues, and the final decision was "It's fine", so go ahead! 😄

[kirrg001]

You can automate the ES6 migration using https://github.com/esnext/esnext. I have recently used it just to try it out and it worked okay. I think the module is definitely not compatible with everything, but wanted to share if somebody wants to try it out 🤠

[mandeepm91]

Sorry about that long list of commit messages. My old branch got messed up while syncing my fork with the upstream. I have kept that old branch aside for now and created a new one. I will be creating small PRs with 2 files in each PR from now onwards

[iamclaytonray]

I'm new but it would be super easy to use TypeScript to access ES6+ features even if you didn't type anything, though, that's always a great idea. I'd be happy to refactor to TS or help with the ES6+ refactor. It would also be great to use named exports with destructured imports. This helps a lot with future contributors because of auto-complete. Also, would classes be on this list instead of prototypes?

(named exports instead of default exports are better, IMO. With defaults, someone could:

import Something from './something';
import something from './something';

[rishabhgrg]

Update: API folders are now being updated by Ghost team.

[naz]

Update: /web folder being worked on by Ghost team :tada:

[m1guelpf]

s the migration complete? The checkboxes seem outdated...

[vikaspotluri123]

I think with the exception of API everything should be up to date 🤔

[kirrg001]

It's not finished. e.g. the data/ folder was not transformed into ES6.

[Jgriebel1990]

I will hop on and contribute to this too

[vikaspotluri123]

@Jgriebel1990 feel free to pick something from the checklist and let us know! I'll probably update the list w/ a more in depth status next week

[ashish-r]

Hi, What's the status. I would like to contribute too.

[naz]

Hey @ashish-r , the honest answer is "you would need to check yourself" :smile: Most of the codebase seems to have been updated, but as @kirrg001 pointed out earlier the data/ folder hasn't been touched, also see that parts of models/ are still on older syntax.

I would suggest updating the list on top first, and then approaching it one piece at a time, so it's easy and fast to merge.

[vikaspotluri123]

Updated list:

• Removed current assignments

  - Removed items that have been migrated

  • [ ] helpers (@vikaspotluri123)
  • [ ] apps (verify)
  • [ ] /amp
  • [ ] /amp/lib/helpers
  • [ ] /amp/lib
  • [ ] /amp/lib/views
  • [ ] /private-blogging
  • [ ] /private-blogging/lib/helpers
  • [ ] /private-blogging/lib
  • [ ] /private-blogging/lib/views
  • [ ] /subscribers
  • [ ] adapters
  • [ ] /scheduling
  • [ ] /scheduling/post-scheduling
  • [ ] /storage
  • [ ] config
  • [ ] /env
• [ ] lib
  
  
  
  • [ ] /fs
  
  
  • [ ] /fs/package-json
  
  
  • [ ] /image
  
  
  • [ ] /mobiledoc/atoms
  
  
  • [ ] /mobiledoc/cards
  
  
  • [ ] /mobiledoc/converters
  
  
  • [ ] /mobiledoc
  
  
  • [ ] /promise
  
  
  • [ ] /security
  
  
  • [ ] /social
  
  
• [ ] data
  
  
  
  • [ ] /db
  
  
  • [ ] /export
  
  
  • [ ] /importer/handlers
  
  
  • [ ] /importer/importers/data
  
  
  • [ ] /importer/importers
  
  
  • [ ] /importer
  
  
  • [ ] /meta
  
  
  • [ ] /migrations/hooks/init
  
  
  • [ ] /migrations/hooks/migrate
  
  
  • [ ] /migrations/init
  
  
  • [ ] /migrations/versions/*/
  
  
  • [ ] /schema
  
  
  • [ ] /schema/clients
  
  
  • [ ] /schema/fixtures
  
  
  • [ ] /validation
  
  
  • [ ] /xml/sitemap
  
  
  • [ ] models
  
  
  • [ ] /base
  
  
  • [ ] /plugins
  
  
  • [ ] /relations
  
  
• [ ] services
  
  
  
  • [ ] /apps
  
  
  • [ ] /auth
  
  
  • [ ] /channels
  
  
  • [ ] /mail
  
  
  • [ ] /mail/templates
  
  
  • [ ] /mail/templates/raw
  
  
  • [ ] /permissions
  
  
  • [ ] /route
  
  
  • [ ] /rss
  
  
  • [ ] /settings
  
  
  • [ ] /themes
  
  
  • [ ] /themes/config
  
  
  • [ ] /url
  
  
• [ ] translations
• [ ] views
• [ ] Tests
• [ ] Root files

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[vikaspotluri123]

Not stale

[ErisDS]

Always really happy to accept PRs that ES6ify the code at any point, but don't think it needs the issue - file structure changes a lot and I think this possibly makes it seem harder?