search

TryGhost / Ghost

Independent technology for modern publishing, memberships, subscriptions and newsletters.
https://ghost.org
MIT License
47.5k stars 10.36k forks source link

ES6 migrations #9589

Closed vikaspotluri123 closed 5 years ago

vikaspotluri123 commented 6 years ago

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

Checklist:

karaggeorge commented 6 years ago

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?

vikaspotluri123 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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

kirrg001 commented 6 years ago

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

After Ghost has dropped it.

vikaspotluri123 commented 6 years ago

Checklist:

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

karaggeorge commented 6 years ago

@vikaspotluri123

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

kirrg001 commented 6 years ago

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 commented 6 years ago

@kirrg001 sounds good!

kirrg001 commented 6 years ago

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.

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

I would like to be involved in this as well.

mikkqu commented 6 years ago

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

kirrg001 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

tiendq commented 6 years ago

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

vikaspotluri123 commented 6 years ago

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

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

kirrg001 commented 6 years ago

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

tiendq commented 6 years ago

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

mandeepm91 commented 6 years ago

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

vikaspotluri123 commented 6 years ago

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

kirrg001 commented 6 years ago

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

kevinansfield commented 6 years ago

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 commented 6 years ago

@vikaspotluri123 I'll start with the api folder

tiendq commented 6 years ago

@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 commented 6 years ago

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

mandeepm91 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

Huc91 commented 6 years ago

@vikaspotluri123 Thanks, I'm gonna take a look

larsnolden commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

naz commented 6 years ago

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

m1guelpf commented 6 years ago

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

vikaspotluri123 commented 6 years ago

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

kirrg001 commented 6 years ago

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

Jgriebel1990 commented 6 years ago

I will hop on and contribute to this too