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

Gruntfile.coffee → Gruntfile.js #885

Merged
merged 7 commits into from
Mar 14, 2017
Merged

Gruntfile.coffee → Gruntfile.js #885

merged 7 commits into from
Mar 14, 2017

Conversation

bennycode
Copy link
Contributor

Type of change

Screenshot

@lipis lipis requested review from lipis, herrmannplatz and gregor March 14, 2017 11:17
Gruntfile.js Outdated

'use strict';

module.exports = grunt => {
Copy link
Contributor

@ffflorian ffflorian Mar 14, 2017

Choose a reason for hiding this comment

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

Let's stick to consistency:

module.exports = (grunt) => {

instead of

module.exports = grunt => {

Like in line 119.

Gruntfile.js Outdated
// Test Related
grunt.registerTask('test', () => grunt.task.run(['clean:docs_coverage', 'scripts', 'test_init', 'test_prepare', 'karma:test']));

grunt.registerTask('test_prepare', function(test_name) {
Copy link
Contributor

@ffflorian ffflorian Mar 14, 2017

Choose a reason for hiding this comment

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

Why not (test_name) => ?

Gruntfile.js Outdated

grunt.registerTask('test_init', ['prepare_dist', 'prepare_test']);

grunt.registerTask('test_run', test_name => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here also

(test_name) => {

Gruntfile.js Outdated
if (target === 'prod' || target === 'staging') {
return grunt.task.run(`set_version:${target}`, 'init', `prepare_${target}`, 'aws_prepare');
} else if (target === 'dev') {
return grunt.task.run("set_version:staging", 'init', "prepare_staging", 'aws_prepare');
Copy link
Contributor

Choose a reason for hiding this comment

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

' instead of "

Gruntfile.js Outdated
const app_files = prepare_file_names(scripts.app);
const component_files = prepare_file_names(scripts.component);
const vendor_files = prepare_file_names(scripts.vendor);
const test_files = test_name ? ["../test/js/#{test_name}Spec.js"] : ['../test/**/*Spec.js'];
Copy link
Contributor

Choose a reason for hiding this comment

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

' instead of "

@lipis
Copy link
Contributor

lipis commented Mar 14, 2017

First of all I'm 100% with the team to move away from CoffeeScript, even though I used to be a big supporter of it and the main influencer for this choice in this project. But times have changed and CoffeeScript is keeping us behind, as we can't use the latest and greatest features of JavaScript.

Having said that, I'm just a little bit against the timing of it. We should probably update the build scripts as a last step, instead of first.

Here is why:

  • The majority of our users/contributors don't care about the build scripts, but the source code
  • We should spend our time on getting rid of CoffeeScript in the source code
  • Once everything is in pure JS we can drop CoffeeScript by updating the build scripts as a final step, because:
    • These files are changed very rarely
    • Almost 100% of these files are consisting of variables, JSON objects and comments
    • CSON is much easier to read/manipulate than JSON and there is no need to declare variables in CoffeeScript
    • Readability for better maintenance is actually the main reasons of CS to exist at all
    • It already works, so no risk involved by updating a very important piece of the puzzle
    • No need to spend valuable resources at the moment
    • If refactoring we should maybe even drop Grunt and switch to Gulp (modern, better, faster)?!

What should we do instead

Since #883 is already merged my vote is to do the following in that order:

  1. All the new files should be written in pure JS
  2. Create a new issue where we point out to all folders in script directory as checkboxes to track the progress
  3. Update Grunt files to JavaScript
  4. Remove CoffeeScript from the dependencies of this project

@bennycode
Copy link
Contributor Author

@lipis, dropping CoffeeScript means dropping CoffeeScript.

It doesn't matter with which files we start our "CoffeeScript to JavaScript" movement because by the end of this movement, there will be no more CoffeeScript.

To start I just picked a file with little logic (which is Gruntfile.coffee). It was very easy to transpile Gruntfile.coffee because it doesn't use much CoffeeScript sugar. The main parts were:

1.

prepare_file_names = (file_name_array) =>
  return (file_name.replace 'deploy/', '' for file_name in file_name_array)

const prepare_file_names = (file_name_array) => {
  return file_name_array.map(file_name => file_name.replace('deploy/', ''));
};

2.

test_files = if test_name then ["../test/js/#{test_name}Spec.js"] else ['../test/**/*Spec.js']

const test_files = test_name ? ['../test/js/#{test_name}Spec.js'] : ['../test/**/*Spec.js'];

Everything else I just throwed into js2.coffee and from there into Lebab. Afterwards I executed eslint Gruntfile.js --fix. Done!

P.S. I know that it hurts letting things go. But in the end it's the birth of something new. 🌻

@lipis
Copy link
Contributor

lipis commented Mar 14, 2017

Sure and I don't mind letting it go.. It's done, dead, kaput, null. All I'm saying is that we just have to attack it from the other side :)

@bennycode bennycode merged commit ebfd566 into dev Mar 14, 2017
@bennycode bennycode deleted the es6-gruntfile branch March 14, 2017 12:56
@ffflorian
Copy link
Contributor

🎉

@lipis lipis mentioned this pull request Mar 14, 2017
79 tasks
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.

3 participants