Support Config.toString() with name hints #53

yyx990803 · 2018-05-12T22:06:37Z

As discussed on Twitter :)

edmorley

Many thanks for the PR! I've wanted this feature for ages!

Here's some example output with a project using Neutrino's react + airbnb presets (from master) + custom overrides:
https://emorley.pastebin.mozilla.org/9085355

Do you have a sense of whether we should call this a breaking change or not? Whilst it's techincally breaking, I can't really believe people we're relying on the previous [object Object] of the inherited .toString().

Also, I wonder if in the future (probably not this PR) we should add mappings to be able to add the imports for the most common plugins/loaders?

edmorley · 2018-05-13T22:31:52Z

src/Config.js

+      if (value && value.__pluginName) {
+        let match = (
+          value.constructor &&
+          value.constructor.toString().match(pluginRE)


What are your thoughts about using .name instead of .toString(), which would mean the regexp could be dropped?

edmorley · 2018-05-13T22:35:07Z

src/Config.js

+        );
+        // special case for copy-webpack-plugin which uses a non-standard constructor
+        if (value.__pluginName === 'copy') {
+          match = [null, `CopyWebpackPlugin`];


I tried a few variations of this to try and find a generic solution for plugins that do this:
https://github.com/webpack-contrib/copy-webpack-plugin/blob/6205f516f7dd027a93737bf533a3be36d57602d9/src/index.js#L174-L175

I couldn't find any that worked once the plugin was already instantiated, however a this.get('plugin').name in Plugin.js worked -- perhaps it might be best to determine the class/function identifier there and pass back along with the name/args?

edmorley · 2018-05-14T00:06:33Z

src/Config.js

+
+        if (name) {
+          return prefix + `new ${name}(${
+            value.__pluginArgs.map(arg => stringify(arg)).join(',\n')


This generates plugin output like:

/* config.plugin('copy') */ new CopyWebpackPlugin([ 'ui/contribute.json', 'ui/revision.txt', 'ui/robots.txt' ], { debug: 'warning' }),

I played around to see if the indentation could be improved, and came up with this convoluted looking thing:

const args = value.__pluginArgs.length ? `\n${value.__pluginArgs.map( arg => indent + stringify(arg).split('\n').join('\n' + indent) ).join(',\n')}\n` : ''; return prefix + `new ${name}(${args})`;

(the intentation stategy taken from: https://github.com/blakeembrey/javascript-stringify/blob/9d2b937ac768d41239ca0088aff90e5ef4e7e113/javascript-stringify.js#L130)

Which gives:

/* config.plugin('copy') */ new CopyWebpackPlugin( [ 'ui/contribute.json', 'ui/revision.txt', 'ui/robots.txt' ], { debug: 'warning' } ),

However I'm open to ideas about whether this is (a) worth it, (b) the right indentation anyway (albeit does match prettier).

I played around to see if the indentation could be improved, and came up with this convoluted looking thing:

Actually, this works and is much cleaner:

// Stringifying the entire args array and discarding the first (`[`) and // last (`]`) characters is simpler than handling each element separately // and then having to manually fix the indentation. const args = stringify(value.__pluginArgs).slice(1, -1); return prefix + `new ${name}(${args})`;

(if you end up using this, feel free to adjust the code comment to read more clearly)

edmorley · 2018-05-14T00:08:48Z

src/Resolve.js

@@ -27,7 +27,7 @@ module.exports = class extends ChainedMap {

  plugin(name) {
    if (!this.plugins.has(name)) {
-      this.plugins.set(name, new Plugin(this));
+      this.plugins.set(name, new Plugin(this, name));


Could you add a test for this one to test/Resolve.js? :-)

edmorley · 2018-05-14T00:10:44Z

test/Config.js

+
+  config
+    .plugin('gamma')
+      .use(class TestPlugin {}, ['foo']);


Could you add a couple more plugins here, to also test the zero and two argument cases?

edmorley · 2018-05-14T01:47:43Z

src/Config.js

+    return stringify(config, (value, indent, stringify) => {
+      // shorten long functions
+      if (!verbose && typeof value === 'function' && value.toString().length > 100) {
+        return `function () { /* omitted long function */ }`;


I wonder whether it might be useful to allow downstream consumers to defineProperties() on functions they pass, so they can override what's output? (In Neutrino's case for the eslint loader formatter option, we'd want to make the output be require('eslint/lib/formatters/codeframe') rather than the function)

edmorley · 2018-05-14T02:10:43Z

src/Config.js

+          match = [null, `CopyWebpackPlugin`];
+        }
+        const name = match[1];
+        const prefix = `/* config.plugin('${value.__pluginName}') */\n`;


Sorry forgot one last thing... I think it might be good to allow customising the identifier used for the config object. eg default to config, but allow Neutrino to override to neutrino.config, which would give: /* neutrino.config.plugin(...) */.

yyx990803 · 2018-05-14T21:31:30Z

Should have addressed everything.

I don't think anyone would find the original config.toString() useful in any sense so I think we should be fine labeling this as non-breaking.

Regarding custom config prefix, I've made config.toString() accept an options object:
```
config.toString({
  verbose: true,
  configPrefix: 'neutrino.config'
})
```

Custom plugin and function stringification: if a plugin constructor or a function has an __expression property, it will be used over its name. This allows us to generate output like:

{
  module: {
    rules: [
      {
        test: require('custom-test')
      }
    ]
  },
  plugins: [
    new (require('copy-webpack-plugin'))()
  ]
}

With these in place we should be able to generate fully usable webpack config!

edmorley

This is amazing! Thank you for the latest changes :-)

@eliperelman do you want to have a quick look before I merge?

edmorley · 2018-05-14T22:12:55Z

src/Config.js

+          const args = stringify(value.__pluginArgs).slice(1, -1);
+          return prefix + `new ${constructorName}(${args})`;
+        } else {
+          return prefix + stringify({ args: value.__pluginArgs || [] });


Are there known cases where this path is taken, or is it for defensive purposes?

Mostly defensive when I found out about the case of copy-webpack-plugin, but this is probably unnecessary as a plugin has to be new-able and thus must have a name.

edmorley · 2018-05-14T22:42:51Z

Ah one last thing - if you have a chance could you add a mention to the README about .toString() and it's options, and also to the individual plugin/rule sections something about __expression? If you don't have time no worries :-)

yyx990803 · 2018-05-15T14:38:32Z

Added readme section.

eliperelman

😍 This in incredible! Thank you so much for your hard work on this one!

eliperelman · 2018-05-15T15:25:05Z

Published in v4.7.0.

edmorley · 2018-06-05T20:22:01Z

I'm not sure this is something worth the extra dependencies of fixing, so not going to file as a new issue for now - but thought worth mentioning it here in case others were similarly confused when trying toString() for the first time.

For plugins that mutate any options objects passed to them as parameters, these mutated options are what will appear in the toString() output, rather than the original arguments.

For example:

const CopyWebpackPlugin = require('copy-webpack-plugin');

neutrino.config
  .plugin('copy')
  .use(CopyWebpackPlugin, [
    [ 'foo' ],
    { debug: false }
  ]);

...results in:

    /* neutrino.config.plugin('copy') */
    new CopyWebpackPlugin(
      [
        'foo',
      ],
      {
        debug: 'warning'
      }
    ),

...where debug is 'warning' not the originally passed false, due to the mutation of options here:
https://github.com/webpack-contrib/copy-webpack-plugin/blob/3ef5b6c57dff58cde124732f1e31b84dc9f793eb/src/index.js#L11

In this specific case cloning the options would be trivial using Object.assign() in a map(), however the data type of elements of args can vary, so we'd have to use something like clone-deep, which may or may not be worth the extra dependency, given that:

this only affects badly behaved plugins
the defaults the plugin sets are actually the settings being used, so perhaps result in more explicit configs anyway

This take advantage of the new webpack-chain `toString()` added in neutrinojs/webpack-chain#53. The output from `neutrino --inspect` now lists the correct plugin declarations and arguments, annotates plugins/loaders with hints about how to reference them in a custom Neutrino config, and supports using `__expression` to override the stringified output when needed. The usage of `deep-sort-object` has been removed since it breaks the new `toString()` comment annotations, and really if sorted output is considered important, it should be handled by webpack-chain itself. Fixes #328. Refs #96.

Support Config.toString() with name hints

8194f23

edmorley mentioned this pull request May 13, 2018

Configure Renovate #54

Merged

edmorley suggested changes May 14, 2018

View reviewed changes

edmorley reviewed May 14, 2018

View reviewed changes

yyx990803 added 2 commits May 14, 2018 17:26

Address review comments for Config.toString()

050838c

Add test for custom configPrefx in config.toString()

571f76f

yyx990803 added 2 commits May 14, 2018 17:40

Allow customizing Plugin expression

b782842

Change fn.toString() to fn.__expression for consistency

3da1767

edmorley approved these changes May 14, 2018

View reviewed changes

Add docs for config.toString()

60c94ec

eliperelman approved these changes May 15, 2018

View reviewed changes

edmorley merged commit 705c2ee into neutrinojs:master May 15, 2018

edmorley mentioned this pull request May 15, 2018

Eject config? #25

Closed

edmorley mentioned this pull request Jun 5, 2018

Improve stringified config output by --inspect neutrinojs/neutrino#928

Merged

edmorley mentioned this pull request Aug 1, 2018

how to use a anonymous plugin for create by myself #75

Closed

vladimyr mentioned this pull request Jan 31, 2019

poi@12 fine tuning ExtensionEngine/boutique#79

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Config.toString() with name hints #53

Support Config.toString() with name hints #53

yyx990803 commented May 12, 2018

edmorley left a comment

edmorley May 13, 2018

edmorley May 13, 2018

edmorley May 14, 2018

edmorley May 14, 2018 •

edited

Loading

edmorley May 14, 2018

edmorley May 14, 2018

edmorley May 14, 2018

edmorley May 14, 2018 •

edited

Loading

yyx990803 commented May 14, 2018 •

edited

Loading

edmorley left a comment

edmorley May 14, 2018 •

edited

Loading

yyx990803 May 14, 2018

edmorley commented May 14, 2018

yyx990803 commented May 15, 2018

eliperelman left a comment

eliperelman commented May 15, 2018

edmorley commented Jun 5, 2018 •

edited

Loading

Support Config.toString() with name hints #53

Support Config.toString() with name hints #53

Conversation

yyx990803 commented May 12, 2018

edmorley left a comment

Choose a reason for hiding this comment

edmorley May 13, 2018

Choose a reason for hiding this comment

edmorley May 13, 2018

Choose a reason for hiding this comment

edmorley May 14, 2018

Choose a reason for hiding this comment

edmorley May 14, 2018 • edited Loading

Choose a reason for hiding this comment

edmorley May 14, 2018

Choose a reason for hiding this comment

edmorley May 14, 2018

Choose a reason for hiding this comment

edmorley May 14, 2018

Choose a reason for hiding this comment

edmorley May 14, 2018 • edited Loading

Choose a reason for hiding this comment

yyx990803 commented May 14, 2018 • edited Loading

edmorley left a comment

Choose a reason for hiding this comment

edmorley May 14, 2018 • edited Loading

Choose a reason for hiding this comment

yyx990803 May 14, 2018

Choose a reason for hiding this comment

edmorley commented May 14, 2018

yyx990803 commented May 15, 2018

eliperelman left a comment

Choose a reason for hiding this comment

eliperelman commented May 15, 2018

edmorley commented Jun 5, 2018 • edited Loading

edmorley May 14, 2018 •

edited

Loading

edmorley May 14, 2018 •

edited

Loading

yyx990803 commented May 14, 2018 •

edited

Loading

edmorley May 14, 2018 •

edited

Loading

edmorley commented Jun 5, 2018 •

edited

Loading