Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

Improve error message when using .tap() on undefined plugin #125

Closed
edmorley opened this issue Jan 27, 2018 · 2 comments · Fixed by #271
Closed

Improve error message when using .tap() on undefined plugin #125

edmorley opened this issue Jan 27, 2018 · 2 comments · Fixed by #271
Assignees
@edmorley
Labels
enhancement help wanted

Comments

@edmorley
Copy link
Member

STR:

  1. yarn add neutrino@8.0.18 @neutrinojs/web@8.0.18
  2. mkdir src
  3. touch contribute.json src/index.js
  4. Create a .neutrinorc.js containing:
module.exports = {
  use: [
    '@neutrinojs/web',
    (neutrino) => {
      neutrino.config
        .plugin('copy')
        .tap(args => {
          args[0] = [
            'contribute.json'
          ];
          return args;
        });
    }
  ]
}
  1. yarn start

Expected:
Error message that makes the mistake clear. eg:
Error: Plugin 'copy' is not defined
(along with a stack trace that points to the correct line in .neutrinorc.js)

Actual:
webpack-chain gives an error message that is hard to track back to a root cause (unclear error message and does not reference a specific line in .neutrinorc.js):

C:\Users\Ed\src\neutrino-test\node_modules\webpack-chain\src\Plugin.js:9
    this.init((Plugin, args = []) => new Plugin(...args));
                                     ^

TypeError: Plugin is not a constructor
    at init (C:\Users\Ed\src\neutrino-test\node_modules\webpack-chain\src\Plugin.js:9:38)
    at Object.toConfig (C:\Users\Ed\src\neutrino-test\node_modules\webpack-chain\src\Plugin.js:38:12)
    at clean.Object.assign.plugins.plugins.values.map.plugin (C:\Users\Ed\src\neutrino-test\node_modules\webpack-chain\src\Config.js:69:59)
    at Array.map (<anonymous>)
    at module.exports.toConfig (C:\Users\Ed\src\neutrino-test\node_modules\webpack-chain\src\Config.js:69:38)
    at ChainAction.Future.chain.chain.chain [as mapper] (C:\Users\Ed\src\neutrino-test\node_modules\neutrino\src\api.js:281:61)
    at ChainAction$resolved [as resolved] (C:\Users\Ed\src\neutrino-test\node_modules\fluture\index.js:805:27)
    at resolved (C:\Users\Ed\src\neutrino-test\node_modules\fluture\index.js:220:19)
    at Immediate.EncaseP2$res [as _onImmediate] (C:\Users\Ed\src\neutrino-test\node_modules\fluture\index.js:1847:7)
    at runCallback (timers.js:791:20)
    at tryOnImmediate (timers.js:751:5)
    at processImmediate [as _immediateCallback] (timers.js:722:5)

The root cause here is that the copy plugin is only defined when command === 'build', and so there is no such plugin to .tap() during yarn start.
@eliperelman
Copy link
Member

I could definitely see how this would be an issue. Technically this issue can arise from not using tap at all:

neutrino.config.plugin('copy');

This is due to the way the .plugin method works:

https://github.com/mozilla-neutrino/webpack-chain/blob/2445476d0d7f444c34060f383c890597ca0d2e8d/src/Config.js#L51-L57

Using these methods creates an entry with that name, and then expects it to be defined. Obviously in the case of conditional plugins, this creates a problem. We do specifically state in the docs about the conditional nature of the plugins:

https://neutrino.js.org/packages/web/#plugins

Note: Some plugins are only available in certain environments. To override them, they should be modified conditionally.

But I agree the error messaging could be better in this scenario. This would have to be done in webpack-chain.

@edmorley
Copy link
Member Author

Yeah I agree this is not expected to work - it's the clearer error message that this issue is about :-)

@edmorley edmorley transferred this issue from neutrinojs/neutrino Nov 20, 2018
@edmorley edmorley self-assigned this Jan 4, 2020
edmorley added a commit that referenced this issue Jul 19, 2020
@edmorley
Improve error message when .tap() used on an undefined plugin
e4692ca 
Previously if `.tap()` was called for a plugin that didn't exist, the
`.tap()` call would succeed, but the `.toConfig()` would fail later with:

`TypeError: Cannot read property '__expression' of undefined`

This scenario can typically occur when trying to customise a plugin
added by a previous preset (where the preset only sometimes adds the
plugin, such as in development), and forgetting to add a conditional
using `.has()` before calling `.tap()`.

Whilst #270 changes that exception to a slightly more useful error
message, it is still only shown at the time of the `.toConfig()`, where
it's much harder to debug the cause.

Now the `.tap()` call itself will show the error:

`Error: Cannot call .tap() on a plugin that has not yet been defined. Call plugin('foo').use(<Plugin>) first`

Whilst this error check does technically break the (unlikely) scenario
of calling `.tap()` in a preset before the plugin is defined, that
ordering was already broken, since the `.tap()`'s args would have
been overwritten by the subsequent preset.

Fixes #125.
@edmorley edmorley mentioned this issue Jul 19, 2020
Merged
edmorley added a commit that referenced this issue Jul 20, 2020
@edmorley
Improve error message when .tap() used on an undefined plugin
1fb2f6d 
Previously if `.tap()` was called for a plugin that didn't exist, the
`.tap()` call would succeed, but the `.toConfig()` would fail later with:

`TypeError: Cannot read property '__expression' of undefined`

This scenario can typically occur when trying to customise a plugin
added by a previous preset (where the preset only sometimes adds the
plugin, such as in development), and forgetting to add a conditional
using `.has()` before calling `.tap()`.

Whilst #270 changes that exception to a slightly more useful error
message, it is still only shown at the time of the `.toConfig()`, where
it's much harder to debug the cause.

Now the `.tap()` call itself will show the error:

`Error: Cannot call .tap() on a plugin that has not yet been defined. Call plugin('foo').use(<Plugin>) first`

Whilst this error check does technically break the (unlikely) scenario
of calling `.tap()` in a preset before the plugin is defined, that
ordering was already broken, since the `.tap()`'s args would have
been overwritten by the subsequent preset.

Fixes #125.
edmorley added a commit that referenced this issue Jul 20, 2020
@edmorley
Improve error message when .tap() used on an undefined plugin (#271)
38ef3c4 
Previously if `.tap()` was called for a plugin that didn't exist, the
`.tap()` call would succeed, but the `.toConfig()` would fail later with:

`TypeError: Cannot read property '__expression' of undefined`

This scenario can typically occur when trying to customise a plugin
added by a previous preset (where the preset only sometimes adds the
plugin, such as in development), and forgetting to add a conditional
using `.has()` before calling `.tap()`.

Whilst #270 changes that exception to a slightly more useful error
message, it is still only shown at the time of the `.toConfig()`, where
it's much harder to debug the cause.

Now the `.tap()` call itself will show the error:

`Error: Cannot call .tap() on a plugin that has not yet been defined. Call plugin('foo').use(<Plugin>) first`

Whilst this error check does technically break the (unlikely) scenario
of calling `.tap()` in a preset before the plugin is defined by a later
preset, that ordering was already broken, since the `.tap()`'s args
would have been overwritten by the subsequent preset.

Fixes #125.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@edmorley edmorley

Labels
enhancement help wanted
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Improve error message when .tap() used on an undefined plugin neutrinojs/webpack-chain
2 participants
@eliperelman @edmorley