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

Add support for Babel #110

Merged
merged 1 commit into from
Jun 10, 2015
Merged

Add support for Babel #110

merged 1 commit into from
Jun 10, 2015

Conversation

jamiebuilds
Copy link
Contributor

Resolves #95

return gulp.src('lib/**/*.js')
.pipe(babel())
.pipe(gulp.dest('dist'));
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure what do make this task, since there's no "build" step in place.

@SBoudrias
Copy link
Member

Any reason to include babelrc file if it's not used?

I think you missed the build step in the package.json file.

@jamiebuilds
Copy link
Contributor Author

re: babelrc. I meant to ask about that actually. Many node packages opt to use Babel's loose mode but we don't really recommend using it unless you understand the caveats. So I removed loose mode from the babelrc.

However the babelrc is really the best place to configure Babel as many people end up using babel in several different ways (ie. gulp-babel, babel/register, etc.).

I was wondering if I should add more options to the babelrc generator to configure common Babel options?

@SBoudrias
Copy link
Member

Ah, I don't think so. I believe the simpler the better in the case of generator-node - at least to start with.

Maybe someone else on the team have another opinion though, I'm open to discussion.

assert.fileContent('test/index.js', 'describe(\'my-module\'');
});
});

describe('node:boilerplate --no-babel', function() {
Copy link
Member

Choose a reason for hiding this comment

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

function () {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spacing seems to be very important to you 😛

Copy link
Member

Choose a reason for hiding this comment

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

Yes it is :)

Copy link
Member

Choose a reason for hiding this comment

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

Luckily these comments won't be necessary once the new generator-node is out ;)

@arthurvr
Copy link
Member

Ah, I don't think so. I believe the simpler the better in the case of generator-node - at least to start with.

👍

@arthurvr
Copy link
Member

Neat work @thejameskyle

@jamiebuilds
Copy link
Contributor Author

Quick question while rebasing. I ran into a spot where <% if (...) { -%>...<% } -%> is used... what does this mean?

See:

var istanbul = require('gulp-istanbul');
<% if (includeCoveralls) { -%>
var coveralls = require('gulp-coveralls');
<% } -%>

@arthurvr
Copy link
Member

@thejameskyle It's explained in the yeoman-generator v0.20 release notes :)

@jamiebuilds
Copy link
Contributor Author

Oh sweet, I'm going to take advantage of that

@arthurvr
Copy link
Member

Need to fix the merge conflict but this looks good to me! 👍

@stevemao
Copy link
Contributor

ping

@jamiebuilds
Copy link
Contributor Author

Updated

},

writing: function () {
this.fs.copy(
this.templatePath('index.js'),
this.destinationPath('lib/index.js')
this.destinationPath('lib/index.js'),
{
Copy link
Member

Choose a reason for hiding this comment

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

this.destinationPath('lib/index.js'), {

Copy link
Member

Choose a reason for hiding this comment

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

@thejameskyle ⬆️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, missed this one. Fixed now

@arthurvr
Copy link
Member

cc @SBoudrias if he has any final thoughts. This LGTM. Thanks @thejameskyle.

@SBoudrias
Copy link
Member

LGTM. Can you squash the commits?

We'll probably be good to release 1.0.0 after this is merged.

@jamiebuilds
Copy link
Contributor Author

Squashed

@arthurvr
Copy link
Member

Thanks @thejameskyle!

arthurvr added a commit that referenced this pull request Jun 10, 2015
@arthurvr arthurvr merged commit 7f311d3 into yeoman:node.next Jun 10, 2015
@jamiebuilds jamiebuilds deleted the tjk/babel2 branch June 10, 2015 18:15
@hemanth
Copy link
Member

hemanth commented Jun 11, 2015

👍

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

6 participants