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

Replace the context of the postcss option function by an argument #31

Closed
MoOx opened this issue Oct 18, 2015 · 3 comments
Closed

Replace the context of the postcss option function by an argument #31

MoOx opened this issue Oct 18, 2015 · 3 comments

Comments

@MoOx
Copy link
Contributor

MoOx commented Oct 18, 2015

The way it's currently handled is non intuitive https://github.com/postcss/postcss-loader/blob/a9cb6997eec444b4f90edd67ac7ec4b5d944cbb1/index.js#L28
What about replacing that .call(this) by just calling it with an argument?
Then in the configuration you can instead show a more clear code like

    postcss: function (webpack) {
        return [
            postcssImport({
                // see postcss-import docs to learn about onImport callback
                // https://github.com/postcss/postcss-import

                onImport: function (files) {
                    files.forEach(webpack.addDependency);
                }.bind(webpack)
            })
        ];
    }

Also fyi, I plan to add a addDependencyTo option to postcss-import in order to be able to write even more simpler and clean boilerplate like

    postcss: function (webpack) {
        return [
            postcssImport({
                addDependencyTo: webpack
            })
        ];
    }

What do you think?

Note: to avoid a breaking change, we might start by changing the doc + allow context and argument at the same time (via .call(this, this)), then in a later major release we replace .call(this, this) by (this).

@MoOx
Copy link
Contributor Author

MoOx commented Oct 18, 2015

Btw, we can even reduce the current boilerplate to

    postcss: function () {
        // The context of this function is the webpack loader-context
        // see: http://webpack.github.io/docs/loaders.html#loader-context

        return [
            postcssImport({
                // see postcss-import docs to learn about onImport callback
                // https://github.com/postcss/postcss-import

                onImport: (files) => files.forEach(this.addDependency);
            })
        ];
    }

@ai
Copy link
Contributor

ai commented Oct 18, 2015

Yeap, this is strange API. Do you want to send a PR to store you name in history? :)

@MoOx
Copy link
Contributor Author

MoOx commented Oct 18, 2015 via email

MoOx added a commit to postcss/postcss-import that referenced this issue Oct 19, 2015
MoOx added a commit that referenced this issue Oct 19, 2015
… webpack

I didn’t add any test yet because:

- it’s a trivial change
- adding test(s) for this trivial change will require to refactor all
the tests in order to be able to have several webpack configs
- I am lazy and already made the appropriate change in postcss-import
that took me some time :)

Close #31
@ai ai closed this as completed in #32 Oct 19, 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

No branches or pull requests

2 participants