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

Decaffeinate NoFlo #640

Merged
merged 215 commits into from Jul 27, 2020
Merged

Decaffeinate NoFlo #640

merged 215 commits into from Jul 27, 2020

Conversation

trustmaster
Copy link
Member

@trustmaster trustmaster commented Jun 13, 2020

Migration from CoffeeScript to ES6. This PR converts:

  • examples
  • spec
  • src
  • root files

Background

CoffeeScript served us well in the past and saved us quite some keystrokes time a while ago, when JavaScript didn't have classes, arrow functions, and other syntactic sugar. And despite the core is written in CoffeeScript, it can be used in ES6, TypeScript, Flow, and other JS flavours without any extra effort.

However, the era of CoffeeScript is over, and most of the JavaScript community is happy with the features that ECMAScript provides today. So, we decided to convert NoFlo to ES6 to open it up to a wider developer community and remove dependency on CoffeeScript.

Breaking changes

After conversion, NoFlo API will remain compatible with all the code that is using NoFlo 1.0+ API, and will continue support modules written in ES6, ES7+, and CS. However, we are going to drop support for some of the pre-1.0 features that we planned dropping anyways:

  1. No WirePattern support. WirePattern has been deprecated since 1.0, and all code using it should be migrated to the latest Process API.

The plan

The idea is to start with core library files situated in src/lib, moving from smaller files with fewer dependencies, to files which depend on them.

After the lib is done, it's the time of spec. Then examples.

With no .coffee code left, dependencies should be cleaned up, and maybe Babel setup should be updated.

The process

Pre-requisites

npm install -g bulk-decaffeinate decaffeinate eslint

References

Conversion instructions

Here is the routine that I follow:

  1. Pick the next .coffee file to convert
  2. Copy its content to Decaffeinate REPL to see what changes to the CoffeeScript code will make the conversion easier. Usually it's adding some return at the end of functions.
  3. Apply these changes to the .coffee file before conversion. Don't forget to run grunt test to make sure nothing is broken by these changes.
  4. Run bulk-decaffeinate convert -f {PATH TO FILE}.coffee
  5. Run grunt test again.
  6. Look at DSXXX suggestions at the top of the converted file, fix them. To test intermediate changes quicker, you can run just nodejs suite: grunt test:nodejs.
  7. Look at eslint-disable at the top of the file. Enable one of the rules, fix the code according to ESLint suggestions, do the same with the next rule.
  8. Don't forget to run the full test suite, including browser suite: grunt test or grunt test:browser.
  9. Remove the .original.coffee file and commit changes.

Contributing

If you'd like to help porting NoFlo to ES6 faster, here is how you can do it:

  1. Fork NoFlo repo and switch to decaffeinate branch
  2. Create your branch based on decaffeinate, select files to work on and post it here in comments, so other people don't do duplicate work
  3. Submit a PR, suggesting a merge from your fork to this branch.

Thanks a lot for helping to make NoFlo more up-to-date 🙏

Known issues

  1. Setting component icon statically (via ComponentName::icon) doesn't work as expected, because there is no easy way to set static properties in es2015. We should revisit whether we need that functionality at all, and fall back to setting those on constructor level only if possible.
  2. PhantomJS browser tests are broken because Phantom doesn't support es2015 profile in Babel. We are going to migrate from PhantomJS to Karma in follow-up PRs.

@trustmaster trustmaster added the WIP Work in progress label Jun 13, 2020
@trustmaster trustmaster self-assigned this Jun 13, 2020
@bergie
Copy link
Member

bergie commented Jul 24, 2020

Generally looks like it is good to go. Test code coverage seems to be broken, so that would be good to fix

@trustmaster
Copy link
Member Author

@bergie I'll have a look at the coverage tool.

How about that issue with static icon and description? I had to disable a test where it sets ComponentName::icon, because I couldn't find a quick and non-ugly way to use static properties in es2015, which would also work with getComponent() style definitions. Do you know where that feature is used and if it's safe to drop?

@bergie
Copy link
Member

bergie commented Jul 24, 2020

I guess we can drop it. Just please document in the ChangeLog

@trustmaster
Copy link
Member Author

@bergie code coverage is fixed. I also converted the examples, so the code is completely decaffeinated after this PR.

Copy link
Member

@bergie bergie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

@bergie bergie merged commit 7e3962d into master Jul 27, 2020
@bergie bergie deleted the decaffeinate branch July 27, 2020 11:54
@bergie bergie added this to the NoFlo 2.0 milestone Dec 8, 2020
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.

None yet

2 participants