Skip to content

Adding Webpack v3 and Babel for ES6 Support#2

Open
greteltrost wants to merge 1 commit intoskilld-labs:8.x-7.xfrom
greteltrost:webpack
Open

Adding Webpack v3 and Babel for ES6 Support#2
greteltrost wants to merge 1 commit intoskilld-labs:8.x-7.xfrom
greteltrost:webpack

Conversation

@greteltrost
Copy link
Copy Markdown

@greteltrost greteltrost commented Mar 27, 2017

Added gulp task scripts and webpack is handled with this task. If everything is ok I will update README to reflect the changes.

Steps to test:

1- install fresh drupal
2- clone repo (github.com/greteltrost/zen) and checkout to 'webpack' branch
3- install new theme from starterkit and enable it
4- test es6 by adding some sample code on js/init.js:

const double = [1,2,3].map((num) => num * 2);

5- build js with webpack by running gulp or NODE_ENV=production gulp for minified version (or only javascript with gulp scripts)
6- check new code, should be transpiled to something that all browsers should be capable to run on /dist/app.js

"use strict";
var double = [1, 2, 3].map(function (num) {
  return num * 2;
});

More info about webpack on https://webpack.js.org/concepts/

@greteltrost
Copy link
Copy Markdown
Author

Note: I added lodash as example, but commented it out on init.js

},
devtool: 'source-map'
};

Copy link
Copy Markdown
Contributor

@pabloguerino pabloguerino Mar 28, 2017

Choose a reason for hiding this comment

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

Also related to #5, we need to handle the NODE_ENV variable here when it gets the value of production and add uglifyjs as a webpack plugin,

something like:

if (process.env.NODE_ENV === 'production') {
    config.plugins.push(
        new webpack.optimize.UglifyJsPlugin({
            compress: {
                screw_ie8: true
            }
        })
    )
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Updated webpack to use env variables and handle file compression.

@andriyun
Copy link
Copy Markdown
Member

Great work!
Thank you @xarturia
I'd like to test and check how it works.

I have no experience with babel. So could you provide some simple steps to check like:

  • install fresh drupal
  • clone repo with this PR changes
  • install new theme from starter kit and enable
  • add few lines of ES6 code to [filename]
  • build js using gulp
  • check new code

Thank you )

@greteltrost
Copy link
Copy Markdown
Author

Updated PR description to add steps for testing and some more info. Let me know if you run into any issues.

Comment thread STARTERKIT/gulpfile.js Outdated
// Build Javascript.
// #################
gulp.task('scripts', [], function () {
return gulp.src('js/init.js')
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Here would be better use options.theme.js variable

Comment thread STARTERKIT/js/script.js
@@ -1,31 +0,0 @@
/**
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why we need to remove script.js file?
This file will be included in libraries definition
https://github.com/skilld-labs/zen/blob/8.x-7.x/STARTERKIT/STARTERKIT.libraries.yml#L15

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it suits us better because init.js makes more sense as its the initialization file

},
output: {
path: __dirname + '/dist',
filename: 'app.js',
Copy link
Copy Markdown
Member

@andriyun andriyun Mar 31, 2017

Choose a reason for hiding this comment

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

How we will include this file to project theme?
Maybe need to add reference to this file in libraries file below https://github.com/skilld-labs/zen/blob/8.x-7.x/STARTERKIT/STARTERKIT.libraries.yml#L15

Btw app.js name has weird semantic.
For me better will be combine/replace this file with original scripts file

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

its the generated output, we add only app.js and we are done

@andriyun
Copy link
Copy Markdown
Member

Need final affection tests of es6 code to project theme.
It can be simple case like bind click event to any dom element etc.

@greteltrost
Copy link
Copy Markdown
Author

Updated scripts task with options.theme.js, Thanks Andriy I missed to update that.
script.js file has been renamed to init.js as it makes more sense to me, because this file will only initialize all packages and classes you may need.
For example, to add the standard behaviors file I added an example:

require('script-loader!./behaviors.js');

This way we can have the old theme.behaviors.js file but parsed by babel (so, with ecmascript6 features)
And you can also create new classes, but this covers way more than what I can write in a few lines:
you can load your own packages (to separate logic between files)

import * from 'sidebar.js'

and then webpack will parse all of them for you and you will get only one file in result (this is NOT concatenation, webpack is way more advanced than that)

@andriyun
Copy link
Copy Markdown
Member

andriyun commented Apr 3, 2017

We still have not included js files to theme :(

Before changes we have script.js file that can be included to theme after comment line https://github.com/skilld-labs/zen/blob/8.x-7.x/STARTERKIT/STARTERKIT.libraries.yml#L15

Now this file is deleted

@xarturia lets add proper changes to STARTERKIT.libraries.yml

@greteltrost
Copy link
Copy Markdown
Author

Fixed bug in gulpfile and added the javascript output to the theme libraries file.

@andriyun
Copy link
Copy Markdown
Member

thank you Gretel
Last step is resolve conflicts

Copy link
Copy Markdown
Contributor

@andypost andypost left a comment

Choose a reason for hiding this comment

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

Codestyle issues, but adopt/exclude gulpfile from sniffers makes sense.
Maybe needs new issue to discus "buatifer" for js (compatiple with drupal.ord requirements)

Comment thread STARTERKIT/.babelrc Outdated
@@ -0,0 +1,3 @@
{
"presets": ["es2015"]
} No newline at end of file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NL

Comment thread STARTERKIT/.gitignore Outdated
styleguide
libraries
*.map
dist No newline at end of file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NL

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

done

Comment thread STARTERKIT/gulpfile.js Outdated
$ = require('gulp-load-plugins')(),
browserSync = require('browser-sync').create(),
del = require('del'),
var gulp = require('gulp'),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This makes drupal code style checker phpcs --standard=Drupal . to fail

@pabloguerino
Copy link
Copy Markdown
Contributor

What are we waiting for here ?
If you need help for testing, please don't hesitate to ask for instructions so we can get this merged and move forward.

@andypost
Copy link
Copy Markdown
Contributor

andypost commented May 9, 2017

  1. My review still not addressed
  2. Drupal core discussion about es6 https://www.drupal.org/node/2809281#comment-12075716

@greteltrost greteltrost changed the title Adding Webpack v2 and Babel for ES6 Support Adding Webpack v3 and Babel for ES6 Support Jul 27, 2017
@pabloguerino
Copy link
Copy Markdown
Contributor

it should be ready now, some of the issues (for the gulpfile) will be solved in a separated task

@pabloguerino
Copy link
Copy Markdown
Contributor

Updated and rebased, ready to be merged. This one is tested on several new projects so we know its safe to get it in.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants