Skip to content

refactor: improved loading algorithm of options and configs #234

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 46 additions & 44 deletions lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,53 +47,55 @@ module.exports = function loader (css, map) {

validateOptions(require('./options.json'), options, 'PostCSS Loader')

const rc = {
path: path.dirname(file),
ctx: {
file: {
extname: path.extname(file),
dirname: path.dirname(file),
basename: path.basename(file)
},
options: {}
}
}

if (options.config) {
if (options.config.path) {
rc.path = path.resolve(options.config.path)
Promise.resolve().then(() => {
if (options.exec || options.parser || options.syntax || options.stringifier || options.plugins) {
Copy link
Member

Choose a reason for hiding this comment

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

rm

Copy link
Member Author

Choose a reason for hiding this comment

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

@michael-ciniawsky why? your ugly algo is don't work as expected

Copy link
Member

Choose a reason for hiding this comment

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

return parseOptions.call(this, options)
}

if (options.config.ctx) {
rc.ctx.options = options.config.ctx
const rc = {
Copy link
Member

Choose a reason for hiding this comment

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

rm

Copy link
Member Author

Choose a reason for hiding this comment

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

@michael-ciniawsky we don't need rc if we don't load config

Copy link
Member

Choose a reason for hiding this comment

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

path: path.dirname(file),
ctx: {
file: {
extname: path.extname(file),
dirname: path.dirname(file),
basename: path.basename(file)
},
options: {}
}
}
}

const sourceMap = options.sourceMap
// Read options from options.config only when options.config is object
if (options.config) {
if (options.config.path) {
rc.path = path.resolve(options.config.path)
}

Promise.resolve().then(() => {
const length = Object.keys(options).length

// TODO
// Refactor
if (!options.config && !sourceMap && length) {
return parseOptions.call(this, options)
} else if (options.config && !sourceMap && length > 1) {
return parseOptions.call(this, options)
} else if (!options.config && sourceMap && length > 1) {
return parseOptions.call(this, options)
} else if (options.config && sourceMap && length > 2) {
return parseOptions.call(this, options)
if (options.config.ctx) {
rc.ctx.options = options.config.ctx
}
}

return postcssrc(rc.ctx, rc.path, { argv: false })
}).then((config) => {
if (!config) config = {}
.then((config) => {
Copy link
Member

Choose a reason for hiding this comment

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

wrong ident

Copy link
Member Author

Choose a reason for hiding this comment

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

@michael-ciniawsky don't understand, please provide example

Copy link
Member

Choose a reason for hiding this comment

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

the code indentation is off :), at least looks it

Copy link
Member Author

Choose a reason for hiding this comment

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

@michael-ciniawsky

return postcssrc(rc.ctx, rc.path, { argv: false })
      .then((config) => {
        // Get `sourceMap` option from loader options when no `map` option in `postcss.config.js`
        if (!config.options.map) config.options.map = options.sourceMap

        return config
      })

Where?

// Get `sourceMap` option from loader options when no `map` option in `postcss.config.js`
if (!config.options.map) config.options.map = options.sourceMap

return config
})
}).then((config) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

config is always object parseOptions return object and postcss-load-config also return object

if (config.file) this.addDependency(config.file)

let sourceMap = config.options.map
let plugins = config.plugins || []
let options = Object.assign({

// Disable override `to` option
if (config.options.to) delete config.options.to
Copy link
Member Author

@alexander-akait alexander-akait May 18, 2017

Choose a reason for hiding this comment

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

Disable override important options from postcss config. it is break build.

// Disable override `from` option
if (config.options.from) delete config.options.from
// Disable override `map` option
if (config.options.map) delete config.options.map

let postcssOption = Object.assign({
Copy link
Member

@michael-ciniawsky michael-ciniawsky Jun 14, 2017

Choose a reason for hiding this comment

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

Why was this renamed? Should be options

Copy link
Member Author

Choose a reason for hiding this comment

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

@michael-ciniawsky because options related to loader (const options = loaderUtils.getOptions(this) || {}), postcssOptions related to postcss options

Copy link
Member

Choose a reason for hiding this comment

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

It works atm due to lexical scope, as long as there is no need to rename it please leave it as is

to: file,
from: file,
map: sourceMap
Expand All @@ -105,20 +107,20 @@ module.exports = function loader (css, map) {

// Loader Exec (Deprecated)
// https://webpack.js.org/api/loaders/#deprecated-context-properties
if (options.parser === 'postcss-js') {
if (postcssOption.parser === 'postcss-js') {
css = this.exec(css, this.resource)
}

if (typeof options.parser === 'string') {
options.parser = require(options.parser)
if (typeof postcssOption.parser === 'string') {
postcssOption.parser = require(postcssOption.parser)
}

if (typeof options.syntax === 'string') {
options.syntax = require(options.syntax)
if (typeof postcssOption.syntax === 'string') {
postcssOption.syntax = require(postcssOption.syntax)
}

if (typeof options.stringifier === 'string') {
options.stringifier = require(options.stringifier)
if (typeof postcssOption.stringifier === 'string') {
postcssOption.stringifier = require(postcssOption.stringifier)
}

// Loader API Exec (Deprecated)
Expand All @@ -132,10 +134,10 @@ module.exports = function loader (css, map) {
}

if (sourceMap && typeof map === 'string') map = JSON.parse(map)
if (sourceMap && map) options.map.prev = map
if (sourceMap && map) postcssOption.map.prev = map

return postcss(plugins)
.process(css, options)
.process(css, postcssOption)
.then((result) => {
result.warnings().forEach((msg) => this.emitWarning(msg.toString()))

Expand Down
11 changes: 5 additions & 6 deletions lib/options.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,11 @@ module.exports = function parseOptions (params) {
else if (Array.isArray(params.plugins)) plugins = params.plugins
else plugins = params.plugins

const options = {}

if (typeof params !== 'undefined') {
options.parser = params.parser
options.syntax = params.syntax
options.stringifier = params.stringifier
const options = {
Copy link
Member Author

Choose a reason for hiding this comment

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

We always have params as object, because we have const options = loaderUtils.getOptions(this) || {}

parser: params.parser,
syntax: params.syntax,
stringifier: params.stringifier,
map: params.sourceMap
Copy link
Member Author

Choose a reason for hiding this comment

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

postcss-load-config return map, we should also return map from parseOptions (consistently and don't misleading for future options).

}

const exec = params && params.exec
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
"docs": "jsdoc2md lib/index.js > LOADER.md",
"pretest": "npm run lint && npm run test:build",
"test": "jest",
"test-only": "npm run test:build && jest && npm run clean",
Copy link
Member

Choose a reason for hiding this comment

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

rm

Copy link
Member Author

Choose a reason for hiding this comment

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

@michael-ciniawsky How can I run only tests?

"test:build": "node test/webpack.build.js",
"posttest": "npm run clean",
"release": "standard-version"
Expand Down