Skip to content

Feature/generate loader#183

Merged
evenstensberg merged 25 commits into
webpack:masterfrom
ianjsikes:feature/generate-loader
Sep 29, 2017
Merged

Feature/generate loader#183
evenstensberg merged 25 commits into
webpack:masterfrom
ianjsikes:feature/generate-loader

Conversation

@ianjsikes

Copy link
Copy Markdown
Contributor

What kind of change does this PR introduce?

Feature

Did you add tests for your changes?
A few

If relevant, did you update the documentation?

Summary

See #179 for feature request.
Adds two new commands, webpack-cli generate-loader and webpack-cli generate-plugin. These commands generate webpack-defaults-compliant starter projects for Webpack loaders and plugins, including basic tests.

Does this PR introduce a breaking change?

No

Other information
The starter projects are based off of @TheLarkInn's example loader repo.

@jsf-clabot

jsf-clabot commented Sep 5, 2017

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@evenstensberg

Copy link
Copy Markdown
Member

Heyo! Thanks for submitting a PR, will review later this week! Awesome work! CC-ing @okonet

@TheLarkInn

Copy link
Copy Markdown
Member

@ianjsikes this looks incredible. Thank you so much for this work. This will be incredibly impactful on the webpack community. 🙇🙇🙇

@okonet

okonet commented Sep 5, 2017

Copy link
Copy Markdown
Contributor

That's great! @ianjsikes do you think it would make sense to add tests for the generator?

@ianjsikes

Copy link
Copy Markdown
Contributor Author

@okonet definitely. I'll get started on that.

@ianjsikes
ianjsikes force-pushed the feature/generate-loader branch from 78349d7 to 4aa82f2 Compare September 6, 2017 06:17
@ianjsikes

Copy link
Copy Markdown
Contributor Author

So in attempting to add tests for the generator, I discovered that anything extending yeoman-generator will not work on node v4 or v5 (including the existing webpack-cli init generator) since yeoman-generator is written and distributed in ES6.

I can't figure out a way to write generator tests that will pass on v4 or v5 since yeoman-generator doesn't seem to work on those versions.

@okonet

okonet commented Sep 6, 2017

Copy link
Copy Markdown
Contributor

Does this mean that the webpack init will also not work on Node v4? This is a blocker then and we need to transpile the code in that case.

Comment thread bin/webpack.js Outdated
const inputConfigPath = path.resolve(process.cwd(), filePaths[0]);

return require('../lib/migrate.js')(inputConfigPath, inputConfigPath);
} else if(argv._.includes('generate-loader')) {

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.

If we were to support v4 this would be a thing that needs change. My self included, have used .includes, but I think that's v4<

@evenstensberg evenstensberg left a comment

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.

Well written. One thing that could be changed, is that some of the paths ( destinationPath, for instance ) could be written to a variable instead and reused.

Comment thread lib/generate-loader/loader-generator.js Outdated
}

writing() {
/**

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.

Single line comment

Comment thread lib/generate-loader/loader-generator.js Outdated
this.destinationPath('src/cjs.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.

Single line comment

Comment thread lib/generate-loader/loader-generator.js Outdated
);

/**
* Examples

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.

Single line comment

@@ -0,0 +1,32 @@
import fs from 'fs';
import { runLoaders } from 'loader-runner';
import Promise from 'bluebird';

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.

This should be at 2nd line

@ianjsikes

Copy link
Copy Markdown
Contributor Author

I made the changes @ev1stensberg requested and opened #184 for fixing the yeoman-generator issue.

@evenstensberg evenstensberg left a comment

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.

Left some comments. The plugin and loader has a lot of the same logic. Maybe we could abstract that so that we could reuse some of the logic instead of re-writing it? Also, sorry for being strict, your PR is awesome, just need to make sure everything is top notch!

Comment thread bin/webpack.js Outdated
argv['display-cached-assets'] = true;
}
if(argv._.includes('init')) {
if(argv._.indexOf('init') > -1) {

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.

Prefer indexOf('init') >= 0

Comment thread bin/webpack.js Outdated

return require('../lib/initialize')(initPkgs);
} else if(argv._.includes('migrate')) {
} else if(argv._.indexOf('migrate') > -1) {

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.

indexOf('migrate') >= 0

Comment thread bin/webpack.js Outdated
const inputConfigPath = path.resolve(process.cwd(), filePaths[0]);

return require('../lib/migrate.js')(inputConfigPath, inputConfigPath);
} else if(argv._.indexOf('generate-loader') > -1) {

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.

indexOf('yourVal') >= 0

const yeoman = require('yeoman-environment');
const LoaderGenerator = require('./loader-generator').LoaderGenerator;

/**

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.

/*
 * Pad 
 * Pad
 */

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Without the double asterisks (/**), it isn't recognized as a JSDoc comment. I could still remove it but you won't get any code hints from your editor.

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.

No, don't remove it. We should follow JSDoc everywhere. ESLint should be setup to enforce that.

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.

Weird, we've used it like the one I've proposed, may need to change everything then?

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.

Yeah, we should change it everywhere then.

Comment thread lib/generate-loader/loader-generator.js Outdated
}

default() {
if (path.basename(this.destinationPath()) !== this.props.name) {

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.

Store this in a variable

Comment thread lib/generate-loader/loader-generator.js Outdated
'\nI will create this folder for you.'
);
mkdirp(this.props.name);
this.destinationRoot(this.destinationPath(this.props.name));

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.

Store destinationPath(this.props.name) in a variable for new contributors to easier know what it is

Comment thread lib/generate-loader/loader-generator.js Outdated
}

writing() {
const copy = (filePath) => {

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.

Leave a comment/ description of what this is doing

Comment thread lib/generate-plugin/plugin-generator.js Outdated
}

default() {
if (path.basename(this.destinationPath()) !== this.props.name) {

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.

Use local var instead

Comment thread lib/generate-plugin/plugin-generator.js Outdated
}

writing() {
const copy = (filePath) => {

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.

Maybe abstract this into a util file so you'd avoid re-declaring the same function twice, if it contains the same logic

@ianjsikes

Copy link
Copy Markdown
Contributor Author

@okonet @ev1stensberg is there anything else that needs to be done on this PR?

@evenstensberg

Copy link
Copy Markdown
Member

I'll have a final look and merge if it looks fine @ianjsikes !

@evenstensberg

Copy link
Copy Markdown
Member

Remove the //eslint-disable's and you're set. I don't believe we'd need to worry about that when initializing a project, because we do not know their eslint config settings

@evenstensberg

Copy link
Copy Markdown
Member

LGTM, let's wait for tests and then merge if everything checks out

@evenstensberg
evenstensberg merged commit e887dba into webpack:master Sep 29, 2017
@evenstensberg

Copy link
Copy Markdown
Member

Thanks @ianjsikes ❤️

@ianjsikes
ianjsikes deleted the feature/generate-loader branch September 29, 2017 22:01
evenstensberg added a commit that referenced this pull request Oct 5, 2017
* add unit tests for everything and flow compilation

* add travis builds

* add cli to signature & add ignore to trash code

* port to jest

* remove redundant cli command

* Feature/generate loader (#183)

* Add template files for loader yeoman generator

* Create yeoman generator for a webpack loader project

* Add tests for loader-generator

* Add `mkdirp` dependency for loader-generator

* Add function to create yeoman env and run loader-generator

* Add `generate-loader` command to webpack-cli

* Copy loader templates from proper directory

* Add template files for plugin generator

* Create yeoman generator for webpack plugins

* Add function to create yeoman env and run plugin generator

* Add cli command to generate plugin

* Register generate- commands in yargs

* Add template files for loader examples and tests

* Copy loader test and example template files in generator

* Add template files for plugin examples and tests

* Copy plugin test and example template files in generator

* Refactor generator file copying, switch .includes with .indexOf in CLI arg parsing

* Change `indexOf('foo') > -1` to `indexOf('foo') >= 0`

* Factor out generator copy utilities into separate module

* Rewrite generators using a function that returns a customized generator class

* Fix linting errors

* Remove //eslint-disable lines from template files

* add unit tests for everything and flow compilation

* add travis builds

* add cli to signature & add ignore to trash code

* port to jest

* remove redundant cli command

* rebase against master
evenstensberg pushed a commit that referenced this pull request Oct 22, 2017
* Add template files for loader yeoman generator

* Create yeoman generator for a webpack loader project

* Add tests for loader-generator

* Add `mkdirp` dependency for loader-generator

* Add function to create yeoman env and run loader-generator

* Add `generate-loader` command to webpack-cli

* Copy loader templates from proper directory

* Add template files for plugin generator

* Create yeoman generator for webpack plugins

* Add function to create yeoman env and run plugin generator

* Add cli command to generate plugin

* Register generate- commands in yargs

* Add template files for loader examples and tests

* Copy loader test and example template files in generator

* Add template files for plugin examples and tests

* Copy plugin test and example template files in generator

* Refactor generator file copying, switch .includes with .indexOf in CLI arg parsing

* Change `indexOf('foo') > -1` to `indexOf('foo') >= 0`

* Factor out generator copy utilities into separate module

* Rewrite generators using a function that returns a customized generator class

* Fix linting errors

* Remove //eslint-disable lines from template files
evenstensberg added a commit that referenced this pull request Oct 22, 2017
* add unit tests for everything and flow compilation

* add travis builds

* add cli to signature & add ignore to trash code

* port to jest

* remove redundant cli command

* Feature/generate loader (#183)

* Add template files for loader yeoman generator

* Create yeoman generator for a webpack loader project

* Add tests for loader-generator

* Add `mkdirp` dependency for loader-generator

* Add function to create yeoman env and run loader-generator

* Add `generate-loader` command to webpack-cli

* Copy loader templates from proper directory

* Add template files for plugin generator

* Create yeoman generator for webpack plugins

* Add function to create yeoman env and run plugin generator

* Add cli command to generate plugin

* Register generate- commands in yargs

* Add template files for loader examples and tests

* Copy loader test and example template files in generator

* Add template files for plugin examples and tests

* Copy plugin test and example template files in generator

* Refactor generator file copying, switch .includes with .indexOf in CLI arg parsing

* Change `indexOf('foo') > -1` to `indexOf('foo') >= 0`

* Factor out generator copy utilities into separate module

* Rewrite generators using a function that returns a customized generator class

* Fix linting errors

* Remove //eslint-disable lines from template files

* add unit tests for everything and flow compilation

* add travis builds

* add cli to signature & add ignore to trash code

* port to jest

* remove redundant cli command

* rebase against master
evenstensberg added a commit that referenced this pull request Oct 22, 2017
* chore: Use prettier (#173)

* build: Add webpack lint settings and style code with eslint --fix

We now use the webpack eslint settings that make
sense in the cli. Also use the new --fix of eslint
to automatically beautify the code

* Add additional .eslintrc to /bin to remove unrelated eslint errors

* build: Add jest-cli as dev dependency

* style: fix lintin errors and coding style according to webpack settings

* build: update dependencies and add package-lock to repo

* feat: Require quotes only in property names that require it

* chore: Add again removed settings by merge conflict solving

* tests: Fix failing test

* chore: Use husky instead of pre-commit

pre-commit package stopped working. See observing/pre-commit#113

husky also requires less boilerplate in the package.json

* [Feature] Added support to Flow inside transformations (#174)

* [FLOW] Implemented interfaces and flow types inside the transformations

* [FLOW] first commit

* Added flwo to two other files

* Reviewed interfaces

* Created different intefaces for expressions

* Fixed rest of trasformations

* More developments

* Updated code after code review

* Fixed failing tests

* Removed yarn file

* Applied linting

* Updated packages

* Applied lint style to the imports due to code review

* docs: Documentation for migrate (#175)

* initial draft of migrate documentation

* formatting

* formatting

* added summary of migrate changes

* maintain consistent usage of user through documentation

* Feature/generate loader (#183)

* Add template files for loader yeoman generator

* Create yeoman generator for a webpack loader project

* Add tests for loader-generator

* Add `mkdirp` dependency for loader-generator

* Add function to create yeoman env and run loader-generator

* Add `generate-loader` command to webpack-cli

* Copy loader templates from proper directory

* Add template files for plugin generator

* Create yeoman generator for webpack plugins

* Add function to create yeoman env and run plugin generator

* Add cli command to generate plugin

* Register generate- commands in yargs

* Add template files for loader examples and tests

* Copy loader test and example template files in generator

* Add template files for plugin examples and tests

* Copy plugin test and example template files in generator

* Refactor generator file copying, switch .includes with .indexOf in CLI arg parsing

* Change `indexOf('foo') > -1` to `indexOf('foo') >= 0`

* Factor out generator copy utilities into separate module

* Rewrite generators using a function that returns a customized generator class

* Fix linting errors

* Remove //eslint-disable lines from template files

* Add unit tests for CLI and add flow compilation (#186)

* add unit tests for everything and flow compilation

* add travis builds

* add cli to signature & add ignore to trash code

* port to jest

* remove redundant cli command

* Feature/generate loader (#183)

* Add template files for loader yeoman generator

* Create yeoman generator for a webpack loader project

* Add tests for loader-generator

* Add `mkdirp` dependency for loader-generator

* Add function to create yeoman env and run loader-generator

* Add `generate-loader` command to webpack-cli

* Copy loader templates from proper directory

* Add template files for plugin generator

* Create yeoman generator for webpack plugins

* Add function to create yeoman env and run plugin generator

* Add cli command to generate plugin

* Register generate- commands in yargs

* Add template files for loader examples and tests

* Copy loader test and example template files in generator

* Add template files for plugin examples and tests

* Copy plugin test and example template files in generator

* Refactor generator file copying, switch .includes with .indexOf in CLI arg parsing

* Change `indexOf('foo') > -1` to `indexOf('foo') >= 0`

* Factor out generator copy utilities into separate module

* Rewrite generators using a function that returns a customized generator class

* Fix linting errors

* Remove //eslint-disable lines from template files

* add unit tests for everything and flow compilation

* add travis builds

* add cli to signature & add ignore to trash code

* port to jest

* remove redundant cli command

* rebase against master

* ast for devServer (#185)

* feat: replace yarn backup w/ normal yarn (#160)

* feat: replace yarn backup w/ normal yarn

* upgrade npm & remove yarn

* Use yarn if available (#189)

* bump version to 1.3.4

* update package.json

* 1.3.5

* prepublish -> postinstall

* 1.3.6

* temp fix to prepublish issue

* 1.3.7

* add propper post install

* 1.3.8

* re-add manual build step

* 1.3.9

* refactor

* bump version

* disable published pkg

* add ignore patterns

* add more ignore patterns for npm
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.

5 participants