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

Add various methods to configure default plugins #152

Merged
merged 3 commits into from Sep 14, 2017

Conversation

Projects
None yet
2 participants
@Lyrkan
Copy link
Collaborator

commented Sep 4, 2017

This PR adds some methods to configure default plugins (closes #148, closes #87 and closes #15):

  • Encore.configureDefinePlugin(callback)
  • Encore.configureExtractTextPlugin(callback)
  • Encore.configureFriendlyErrorsPlugin(callback)
  • Encore.configureLoaderOptionsPlugin(callback)
  • Encore.configureUglifyJsPlugin(callback)

Other changes:

  • Allows the clean-webpack-plugin to be configured using Encore.cleanupOutputBeforeBuild(paths, callback)
  • Fixed a small mistake in the Encore.configureManifestPlugin() jsdoc
  • Reorganized flags/callbacks in the constructor of webpack.config.js since it was starting to be a bit hard to read. I'm not sure about the way I splitted things though, let me know what you think about it.
  • Renamed common-chunks.js to commons-chunks.js since the name of the plugin is CommonsChunkPlugin

@weaverryan: Not directly related but while doing all of that I noticed that sassOptions uses snake-case whereas I used camel-case in #144 for enablePreactPreset() and in #115 for configureRuntimeEnvironment(), should we do something about that?

Edit 1: Added Encore.configureCleanWebpackPlugin()
Edit 2: Removed Encore.configureCleanWebpackPlugin() to use Encore.cleanupOutputBeforeBuild(paths, callback) arguments instead

@Lyrkan Lyrkan force-pushed the Lyrkan:configure-plugins branch from 51bf683 to 7f1fcbd Sep 4, 2017

@Lyrkan Lyrkan force-pushed the Lyrkan:configure-plugins branch from 7f1fcbd to 90c8565 Sep 4, 2017

@Lyrkan

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 12, 2017

Just realized that maybe I should remove configureCleanWebpackPlugin and move the paths and callback arguments to cleanupOutputBeforeBuild instead...

@weaverryan

This comment has been minimized.

Copy link
Member

commented Sep 13, 2017

Not directly related but while doing all of that I noticed that sassOptions uses snake-case whereas I used camel-case in #144 for enablePreactPreset() and in #115 for configureRuntimeEnvironment(), should we do something about that?

Hmm, yea, good catch. We should probably "get this right" now. I'm leaning towards using camelCase... so we would "fix" sassOptions (I'm leaning towards this in part because the options in #115 are camelCase just because that's how the CLI options are normalized internally.

@weaverryan
Copy link
Member

left a comment

Minor changes. This all looks really nice. I think we will ultimately have a lot of these configure* methods... but I kind of like that: it's easy to see what you can configure.

return this;
},

/**

This comment has been minimized.

Copy link
@weaverryan

weaverryan Sep 13, 2017

Member

This all looks beautiful

index.js Outdated
@@ -522,10 +618,20 @@ const publicApi = {
* If enabled, the output directory is emptied between
* each build (to remove old files).

This comment has been minimized.

Copy link
@weaverryan

weaverryan Sep 13, 2017

Member

It's not from your commit, but this could probably fit all on one line.

*
* For example:
*
* Encore.cleanupOutputBeforeBuild(['*.js'], (options) => {

This comment has been minimized.

Copy link
@weaverryan

weaverryan Sep 13, 2017

Member

Maybe we should show the default ['**/*'] as the first option. The issue is that, if I only want to specify some options for the plugin... I need to know what this first value is by default, so I can repeat it. That kind of sucks... but I can't think of a good way around it (so showing the default here might make sense)

This comment has been minimized.

Copy link
@Lyrkan

Lyrkan Sep 13, 2017

Author Collaborator

I wanted to but '**/*' breaks the multi-line comment :(

This comment has been minimized.

Copy link
@weaverryan

weaverryan Sep 14, 2017

Member

Ha! What a crazy edge case :p

cleanupOutputBeforeBuild() {
cleanupOutputBeforeBuild(paths = ['**/*'], cleanWebpackPluginOptionsCallback = () => {}) {
if (!Array.isArray(paths)) {
throw new Error('Argument 1 to cleanupOutputBeforeBuild() must be an Array');

This comment has been minimized.

Copy link
@weaverryan

weaverryan Sep 13, 2017

Member

What do you think about this:

... must be an Array of paths - e.g. ['**/*']

@weaverryan

This comment has been minimized.

Copy link
Member

commented Sep 14, 2017

Thank you @Lyrkan!

@weaverryan weaverryan merged commit 286787a into symfony:master Sep 14, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

weaverryan added a commit that referenced this pull request Sep 14, 2017

feature #152 Add various methods to configure default plugins (Lyrkan)
This PR was squashed before being merged into the master branch (closes #152).

Discussion
----------

Add various methods to configure default plugins

This PR adds some methods to configure default plugins (closes #148, closes #87 and closes #15):

* `Encore.configureDefinePlugin(callback)`
* `Encore.configureExtractTextPlugin(callback)`
* `Encore.configureFriendlyErrorsPlugin(callback)`
* `Encore.configureLoaderOptionsPlugin(callback)`
* `Encore.configureUglifyJsPlugin(callback)`

Other changes:

* Allows the `clean-webpack-plugin` to be configured using `Encore.cleanupOutputBeforeBuild(paths, callback)`
* Fixed a small mistake in the `Encore.configureManifestPlugin()` jsdoc
* Reorganized flags/callbacks in the constructor of `webpack.config.js` since it was starting to be a bit hard to read. I'm not sure about the way I splitted things though, let me know what you think about it.
* Renamed `common-chunks.js` to `commons-chunks.js` since the name of the plugin is `CommonsChunkPlugin`

@weaverryan: Not directly related but while doing all of that I noticed that `sassOptions` uses snake-case whereas I used camel-case in #144 for `enablePreactPreset()` and in #115 for `configureRuntimeEnvironment()`, should we do something about that?

***Edit 1:** Added `Encore.configureCleanWebpackPlugin()`*
***Edit 2:** Removed `Encore.configureCleanWebpackPlugin()` to use `Encore.cleanupOutputBeforeBuild(paths, callback)` arguments instead*

Commits
-------

286787a Minor text changes
f72614b Remove configureCleanWebpackPlugin and use cleanupOutputBeforeBuild arguments instead
90c8565 Add various methods to configure default plugins

@Lyrkan Lyrkan deleted the Lyrkan:configure-plugins branch Sep 14, 2017

weaverryan added a commit that referenced this pull request Sep 14, 2017

minor #159 Replace "resolve_url_loader" by "resolveUrlLoader" (Lyrkan)
This PR was squashed before being merged into the master branch (closes #159).

Discussion
----------

Replace "resolve_url_loader" by "resolveUrlLoader"

This PR deprecates the `resolve_url_loader` option used by the `enableSassLoader` and replaces it by `resolveUrlLoader`.

This is done to be consistent with other methods that use camel-case instead of snake-case (see [#152](#152 (comment))).

`resolve_url_loader` will still work for now but will display the following deprecation message:

![2017-09-13_23-38-56](https://user-images.githubusercontent.com/850046/30402523-4c736d86-98de-11e7-97c1-81a216912fb7.png)

Commits
-------

ff442a6 Add a logger.deprecation(message) method
9dac153 Replace sass option 'resolve_url_loader' by 'resolveUrlLoader'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.