Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Decaffeinate - #48

Merged
sadick254 merged 43 commits into
atom:masterfrom
aminya:decaffeinate
Oct 20, 2020
Merged

Decaffeinate#48
sadick254 merged 43 commits into
atom:masterfrom
aminya:decaffeinate

Conversation

@aminya

@aminya aminya commented Jul 12, 2020

Copy link
Copy Markdown
Contributor

This pull requests updates fs-plus without changing the API:

  • decaffeinate: src code is converted to Modern JavaScript
  • Rollup: fs-plus is built with rollup which makes it load much faster
  • babel: uses babel for conversion to cjs
  • Enhances the performance and loading time

Verification Process

Tests are untouched to show backward compatibility! As you see all of them pass.

Applicable Issues

Release Notes

  • decaffeinate: src code is converted to Modern JavaScript
  • Rollup: fs-plus is built with rollup which makes it load much faster
  • babel: uses babel for conversion to cjs
  • Enhances the performance and loading time

@aminya
aminya force-pushed the decaffeinate branch 3 times, most recently from e42b830 to d89dfc3 Compare July 12, 2020 11:45
@aminya
aminya force-pushed the decaffeinate branch 4 times, most recently from f97a173 to cbcb6c3 Compare August 29, 2020 06:59
@aminya
aminya marked this pull request as ready for review August 29, 2020 07:12
@aminya

aminya commented Aug 29, 2020

Copy link
Copy Markdown
Contributor Author

@sadick254 This is ready now

@aminya aminya mentioned this pull request Aug 29, 2020
@aminya

aminya commented Sep 27, 2020

Copy link
Copy Markdown
Contributor Author

@sadick254 @darangi Could you take a look at this? I have some plans in my mind to improv fs-plus, which depend on this.

@sadick254

Copy link
Copy Markdown
Contributor

@aminya Could you investigate what is causing the node 14 failure on appveyor

@aminya

aminya commented Oct 12, 2020

Copy link
Copy Markdown
Contributor Author

@sadick254 That is just a one-time thing that happens in Appveyor sometimes. Restart the failing jobs and it will go away.

If you check it has not even started
image

@aminya

aminya commented Oct 19, 2020

Copy link
Copy Markdown
Contributor Author

@sadick254 Are you planning to merge this?

I have big plans for fs-plus. I want to reexprt the missing methods from fs-extra, so we can just use fs-plus everywhere.

Just as a reference, Atom maintainers were more open to decaffeination, but this seems to have changed for unknown reasons. For example, see this PR which was merged in a day atom/event-kit#38

@sadick254

Copy link
Copy Markdown
Contributor

Hey @aminya,
We are still very much open to decaffeination. This PR will be merged.

Just as a reference, Atom maintainers were more open to decaffeination, but this seems to have changed for unknown reasons. For example, see this PR which was merged in a day atom/event-kit#38

Right now we are quite a small team with a lot to do. We might not be able to merge PR's in a day.

@sadick254

Copy link
Copy Markdown
Contributor

@aminya When decaffeinating please avoid introducing other changes. You should create separate PR's for the changes unrelated to decaffeination.

@aminya

aminya commented Oct 20, 2020

Copy link
Copy Markdown
Contributor Author

I did not do that. I have actually kept my changes for future PRs.

These commits that you see are my manual work to make decaffeination more appealing. When you use the decaffeinate tool, it generates some suggestions. I just applied those so the code looks succinct. These recommendations are usually these:

  • Remove implicit returns: remove unnecessary return in functions because of CoffeeScript
  • Remove inline assignment: move assignment out of if the head
  • Shorter null check: using if (variable) instead of if (variable != null) whenever it is safe
  • Remove IIFE: unnecessary returns because of CoffeeScript
  • Remove unnecessary Array.from
  • Remove guard: using if checks instead of optional chaining in CoffeeScript (generated if not using optional chaining) => Instead, we use a new babel version with optional chaining enabled.
    ...

If you follow my commit messages, this is what I have done.

@sadick254

Copy link
Copy Markdown
Contributor

I am ok with the suggestions. In this PR i see the introduction to rollup. This could have been a separate PR. Just wanted to bring it to your attention.

@aminya

aminya commented Oct 20, 2020

Copy link
Copy Markdown
Contributor Author

I can move it to another PR if it helps. That is not a problem at all. We can use a new babel version by itself too.

@sadick254

Copy link
Copy Markdown
Contributor

No need. I have already reviewed the changes and happy to merge it as it is.

@sadick254
sadick254 merged commit e552f94 into atom:master Oct 20, 2020
@aminya

aminya commented Oct 20, 2020

Copy link
Copy Markdown
Contributor Author

Awesome! Thanks for merging.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants