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

Support extract-text for styles via options.extract #443

Merged
merged 2 commits into from Nov 17, 2017

Conversation

timkelty
Copy link
Contributor

@timkelty timkelty commented Nov 16, 2017

Fixes #439

Notes:

@timkelty timkelty changed the title WIP: Support extract-text for styles via options.extract Support extract-text for styles via options.extract Nov 16, 2017
module.exports = ({ config }, options = {}) => config.module
const ExtractTextPlugin = require('extract-text-webpack-plugin');
const merge = require('deepmerge');
const { pick, mapValues } = require('lodash');
Copy link
Member

@eliperelman eliperelman Nov 16, 2017

Choose a reason for hiding this comment

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

We typically use ramda in place of lodash, so let's swap that out here, or use native implementations where possible.

}));

options.extract.loader = Object.assign({
use: Object.values(pick(loadersByKey, useKeys)),
Copy link
Member

@eliperelman eliperelman Nov 16, 2017

Choose a reason for hiding this comment

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

Object.values isn't available in Node.js v6, so you'll have to map the values out:

Object.keys(object).map(key => object[key])

}, options.extract.loader || {});

styleRule
.uses
Copy link
Member

@eliperelman eliperelman Nov 16, 2017

Choose a reason for hiding this comment

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

Add 2 more spaces of indentation.

.when(options.cssHotLoader, use => use.options(options.cssHotLoader));
});

ExtractTextPlugin.extract(options.extract.loader)
Copy link
Member

@eliperelman eliperelman Nov 16, 2017

Choose a reason for hiding this comment

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

Move .extract to the next line, and increase its indent 2 spaces.

ExtractTextPlugin.extract(options.extract.loader)
.forEach(({ loader, options }) => {
styleRule
.use(loader)
Copy link
Member

@eliperelman eliperelman Nov 16, 2017

Choose a reason for hiding this comment

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

Indent 2 more spaces.

.options(options);
});

neutrino.config.plugin('extract')
Copy link
Member

@eliperelman eliperelman Nov 16, 2017

Choose a reason for hiding this comment

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

Move .plugin to the next line and increase indent 2 spaces from the line above it.

const isBuild = neutrino.options.command === 'start';
const options = merge({
styleUseId: 'style',
cssHotLoaderUseId: 'cssHotLoader',
Copy link
Member

@eliperelman eliperelman Nov 16, 2017

Choose a reason for hiding this comment

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

How would you feel about this just being hot, similar to the hot plugin?

Copy link
Contributor Author

@timkelty timkelty Nov 17, 2017

Choose a reason for hiding this comment

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

Sure. Do you mean the the option name (cssHotLoaderUseId) or the value 'cssHotLoader'?

Not sure if we even need it configurable as an option, was just following suit with options.styleUseId, options.cssUseId

@timkelty
Copy link
Contributor Author

@timkelty timkelty commented Nov 17, 2017

I think I've tidied up all the indentation from the review…the only one I wasn't sure about was > Move .extract to the next line, and increase its indent 2 spaces.

This isn't a ChainedMap, so I wasn't sure why we'd break and indent…ExtractTextPlugin.extract() → array

@timkelty
Copy link
Contributor Author

@timkelty timkelty commented Nov 17, 2017

I refactored without lodash or ramda. If we end up wanting ramda, we'll probably just want { pick, map };

@timkelty
Copy link
Contributor Author

@timkelty timkelty commented Nov 17, 2017

I'm now passing options.hot directly to css-hot-loader (if it isn't just true), which is I think what you were getting at.

I'm also now passing options.hot from preset-web.

Copy link
Member

@eliperelman eliperelman left a comment

This is great, I'm so happy to see this! Just one small nit, and we can merge!

.clear()
.end()
.when(options.hot, (rule) => {
rule.use(options.cssHotLoaderUseId || 'cssHotLoader')
Copy link
Member

@eliperelman eliperelman Nov 17, 2017

Choose a reason for hiding this comment

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

Yeah, my comment before wasn't very clear, my apologies. I was asking to name this ID from cssHotLoader to just hot, just the HotPlugin does in other middleware.

@@ -24,6 +24,9 @@ module.exports = (neutrino, opts = {}) => {
env: [],
hot: true,
html: {},
style: {
hot: opts.hot !== false
Copy link
Member

@eliperelman eliperelman Nov 17, 2017

Choose a reason for hiding this comment

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

😍

@@ -13,7 +13,7 @@ module.exports = ({ config }, options = {}) => {

config.module
.rule('img')
.test(/\.(png|jpg|jpeg|gif)(\?v=\d+\.\d+\.\d+)?$/)
.test(/\.(png|jpg|jpeg|gif|webp)(\?v=\d+\.\d+\.\d+)?$/)
Copy link
Member

@eliperelman eliperelman Nov 17, 2017

Choose a reason for hiding this comment

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

Looks like the master commit got re-included here. You may want to:

git checkout master
git pull upstream master
git checkout style-loader-extract
git checkout HEAD~1
git rebase master
git commit -m "Pass options.hot instead of options.cssHotLoader" --amend
git push -f origin style-loader-extract

Copy link
Contributor Author

@timkelty timkelty Nov 17, 2017

Choose a reason for hiding this comment

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

Whups - yeah I rebased from master

@eliperelman
Copy link
Member

@eliperelman eliperelman commented Nov 17, 2017

@timkelty looks like the same thing happened to yargs 🤣

@timkelty
Copy link
Contributor Author

@timkelty timkelty commented Nov 17, 2017

@eliperelman ok, should just have commits for this PR now.

Will this be for 8.x then? (currently would be breaking, only because options.extract defaults to true)

@eliperelman
Copy link
Member

@eliperelman eliperelman commented Nov 17, 2017

Yep! Extraction itself is also breaking, since it would modify hashes of file output.

@timkelty
Copy link
Contributor Author

@timkelty timkelty commented Nov 17, 2017

Last thing, just for future reference - is this how you wanted this bit indented? https://github.com/timkelty/neutrino-dev/blob/10ca3bca6615bb8dae7bafe85dcdcd9a435d3586/packages/style-loader/index.js#L53-L60

I didn't expect you to want the .extract indented as it isn't a ChainedMap changing contexts like the others.

@eliperelman
Copy link
Member

@eliperelman eliperelman commented Nov 17, 2017

@timkelty yes, my indentation preferences just rely on readability, method chaining, and context.

@timkelty
Copy link
Contributor Author

@timkelty timkelty commented Nov 17, 2017

Yikes, that fell through the cracks! b777bb5

@eliperelman eliperelman merged commit 5d509fa into neutrinojs:master Nov 17, 2017
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants