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

Webpack-CLI version 1 #105

Merged
merged 101 commits into from
May 7, 2017
Merged

Webpack-CLI version 1 #105

merged 101 commits into from
May 7, 2017

Conversation

evenstensberg
Copy link
Member

@evenstensberg evenstensberg commented Apr 7, 2017

Webpack CLI version 1

  • AST's

  • Prettier

  • Windows support

  • Documentation

  • Tests

  • Generator with adapter for us later to use for path validation before sending to AST's

const transformsObject = {
entryTransform,
outputTransform
};
Copy link

Choose a reason for hiding this comment

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

This should be passed to runTransform somehow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Doing through p-lazy I suppose ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't review yet, will notify when there's something better to review, this isn't nearly done at all

if(!webpackProperties['output']) {
return ast;
} else if(webpackProperties['output'].length) {
throw new Error('Supplying output with only no options is not supported.');
Copy link
Member Author

Choose a reason for hiding this comment

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

Remind me to change this l8tr :)

@pksjce
Copy link

pksjce commented Apr 9, 2017

Awesome work @ev1stensberg
^following

Thanks to Sindre Sørhus for this fix. Object references are now making
sure our callbacks are held, and removed the return of the promise and
rather used the object we want to return.
@@ -6,44 +6,32 @@ module.exports = (self, answer) => {

let entryIdentifiers;
let entryProperties;
if(answer['entryType'].indexOf('Array') >= 0) {
if(answer['entryType'] == 'Multiple') {
Copy link
Contributor

Choose a reason for hiding this comment

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

== -> ===

}
resolve(webpackEntryPoint);
});
});
}
else {
self.prompt([
Input('singularEntry', 'What\'s your entry point?')
Input('singularEntry', 'Write the location of module you would like to use')
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure write is a good verb here. I'd say type?

this.configuration.config.webpackOptions.module = getDefaultLoaders();
this.configuration.config.webpackOptions.plugins = getDefaultPlugins();
this.prompt([
RawList('entryType', 'What kind of entry point do you want?', ['Array', 'Function', 'String', 'Object'])
RawList('entryType', 'Would you like to bundle a single or multiple javascript modules?',
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put each argument on a new line

@pksjce
Copy link

pksjce commented May 5, 2017

Hey,
Lot's of work done on this! Great to have more eyes on this code!

@ev1stensberg - I checked out the latest and ran webpack-cli --init to see usage. I have a few small concerns.
Here is the screenshot.
screen shot 2017-05-05 at 10 54 56 pm

  1. Asking to add "Remember to add string quotes to your bundle entries and file extensions!" is a bit much. Since it's only an init tool, we need not support too many complex options. We can just assume that what the user has entered is a string. Don't you think so?
  2. " Would you like to bundle a single or multiple javascript modules?" could be simplfied to "Will you be creating multiple bundles?" with "Yes" or "No" answer.
  3. "Type the location of module you would like to use" . This is a bit vague and we need to use simpler terminology.
  4. Once all the prompts are answered, before the npm install runs, lets show a message saying "Created config, installing dependencies..." or something like that?

But, I would suggest an alternate set of questions to ask. The script would be something like

Welcome to webpack! 
We will ask you a few questions to create your configuration file.

1. Enter the name of your entry file - index.js
2. Enter the path for the output file - dist/bundle.js
3. Do you use ES6 in your project? Y/n
4. Do you use one of the below CSS solutions?
a. SASS
b. LESS
c. PostCss
5. Do you want to bundle the CSS to a separate file? Y/n

Creating "webpack.config.js"

module.exports = {
...
}

Do you want to save this file and install dependencies? Y/n
Installing dependencies....
..
..

Congratulations! Your configuration file has been created at `webpack,config.js`

I feel we have the ast support to convert all the above questions to a configuration. This should suffice to be the default init functionality.

Plugin packages could extend these questions to include framework/library specific questions like preact/react etc.

@evenstensberg
Copy link
Member Author

evenstensberg commented May 5, 2017

@pksjce Hi! Great to have you reviewing!

Here's my replies to your concerns:

  1. I feel like we should warn the user, or at least mention it in the docs somewhere. I don't really want to change the entries section, unless it's really needed. The reason is that we are with this logic allowing:
  • CommonChunkPlugin on multiple entries by default
  • Tooltip for CommonsChunk as well, to help users get a notion of perf, continued from a discussion and mention of this with @addyosmani
  • Super dynamic scaffolding for entries, people can add all sorts of types, and reminding people to add quotes is a small disadvantage.

We can, however, try to implement the string addition and remove this burden from the users. Not guaranteeing it will work 100%, but I'll try implementing it along with file-extensions for single entries. I was thinking about consistency when adding the file end note.

  1. I completely agree, will proceed to replace
  2. Yes, same here
  3. Agree on this point too, this is consistent with migrate and allows people to reconsider.

For the CSS part, will refactor. Do you have a chance to review the ast part too?

Edit: I'm not sure if beginners know what those are, we should stick with CSS only and ask for feedback from the community, perhaps?

A question for me is: What is missing after the fixes mentioned to merge and push v1?

@evenstensberg
Copy link
Member Author

A small note to add is that migrate uses a task runner for a single webpack.config. I'm a bit worried about this, as we support multiple package scaffolds in init, which will make the terminal polluted. Perhaps we should ask if the user wants to write the file instead? Can try to integrate the task-runner, but again, no promises. Fixed what you mentioned, except the CSS part, where we need to discuss.

Typing entry now lets you add things such as Array, Function, ArrowFunction, String and properties on strings that aren't litteral, such as path.join or process.cwd().

@addyosmani
Copy link

Fwiw, the proposal by @pksjce in "But, I would suggest an alternate set of questions to ask. The script would be something like" sounds like it would tweak the prompts to be a little more beginner friendly. Perhaps 'Do you use ES2015 in your project?' instead of ES6 :)

Silly question: why wouldn't the user want to create their Webpack config in the last step of 0ba9d7d? It seems extraneous to me. At least with Yeoman generators, the UX we typically use is asking questions and on the final question then kicking of writing to disk.

Reading through @ev1stensberg's work in c58f793, I do like the idea of minimal boilerplate for encouraging the use of common vendor chunks and ExtractTextPlugin.

Another silly q: To what extent if any should we be switching over to babel-minify if the user opts for ES2015 support with Babel? Are we encouraging ES2015 -> ES5 -> Uglify that code?

@evenstensberg
Copy link
Member Author

evenstensberg commented May 7, 2017

@pksjce @addyosmani Corrected the generator as of the comments. The last thing left is the CSS/SASS/LESS prompts, do we want those? I'm not sure, but open for input. Removed the confirm, even though I side with @pksjce on the confirm part, as this is consistent with migrate.

Also, I think we should enable extractTextPlugin by default and skip the bundle CSS in one file, this is better for perf, yes?

@evenstensberg
Copy link
Member Author

Also, heard some things about babel-minify resulting in a bigger bundle, or something in that alley. Pinging @kdzwinel for his input.

@evenstensberg evenstensberg merged commit 302200d into master May 7, 2017
@kdzwinel
Copy link

kdzwinel commented May 7, 2017

Yeah, we did some tests couple of weeks back - babili wasn't terribly bigger (~10%), but it was way slower than babel+uglify.

Babel + uglify:
23.87s - 1.06 MB

Babili:
49.24s - 1.18 MB

This may have changed in the meantime though (babili is a beta project after all).

@addyosmani
Copy link

cc @hzoo as an fyi on the babili part of the discussion

👍 for the current status of this work landing @ev1stensberg

Also, I think we should enable extractTextPlugin by default and skip the bundle CSS in one file, this is better for perf, yes?

👍

@evenstensberg evenstensberg deleted the feature/init-ast branch May 8, 2017 13:16
@anyulled
Copy link

migrate.md file is empty. Is it intentional?

@evenstensberg
Copy link
Member Author

@anyulled Yep, left it for @okonet or @pksjce , but this could be a good first contribution!

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.

7 participants