grafana/toolkit: Add option to override webpack config #20872

sebimarkgraf · 2019-12-04T15:24:31Z

What this PR does / why we need it:
Implement possible option for #20833 to allow user to override webpack configuration.
This is a WIP and is intended to show a possible implementation

Fixes #20833

dprokop · 2019-12-05T21:31:41Z

@Sintifo thanks for this. I'm gonna take a look at this tomorrow morning and come back with some feedback

stevenvachon · 2019-12-06T03:51:58Z

@Sintifo my comment is awaiting an answer. This PR may not be the best approach.

sebimarkgraf · 2019-12-06T09:33:56Z

@stevenvachon I updated the PR to your suggested approach.

This allows the user to implement the getWebpackConfig function themselves, while keeping the possibility to change the configuration depending on the options.

Example:

import { getWebpackConfig as originalGetter, WebpackConfigurationGetter } from '@grafana/toolkit/src/config'
import WorkerPlugin from 'worker-plugin';

export const getWebpackConfig: WebpackConfigurationGetter = (options) => {
    const webpackConfig = originalGetter(options);
    console.log('Custom config');

    webpackConfig.plugins.push(new WorkerPlugin())

    return webpackConfig;
}

I am not sure how you generate the index.ts for top-level imports. When trying to build the toolkit I didn't get any in the dist directory.

I thought about using dynamic await import but sticked to require since that seems to be the way is was done everywhere else.

dprokop · 2019-12-06T11:17:30Z

Regarding this API:

export const getWebpackConfig: WebpackConfigurationGetter = (options) => {

I think this function should get a default toolkit's webpack config object as an argument so that you don't need getWebpackConfig exposed from toolkit.

sebimarkgraf · 2019-12-06T12:54:41Z

@dprokop
Changed to your suggestion. Is it wise to use a default export instead to remove the need for correct naming?

import { CustomWebpackConfigurationGetter } from '@grafana/toolkit/src/config'
import WorkerPlugin from 'worker-plugin';

export const getWebpackConfig: CustomWebpackConfigurationGetter = (defaultConfig, options) => {
    console.log('Custom config');
    
    defaultConfig.plugins.push(new WorkerPlugin())

    return defaultConfig;
}

dprokop · 2019-12-09T08:14:27Z

Changed to your suggestion. Is it wise to use a default export instead to remove the need for correct naming?

What do you mean, the path @grafana/toolkit/src/config? We don't use default exports in our packages. For this particular case I would stick with the full path import as you did now. The only thing I would introduce is the index.ts file under '@grafana/toolkit/src/config' path, so that you don't need to import from @grafana/toolkit/src/config/webpack.plugin.config.

dprokop · 2019-12-09T08:21:09Z

packages/grafana-toolkit/src/config/webpack.plugin.config.ts

+  const customWebpackPath = path.resolve(process.cwd(), 'webpack.plugin.config.ts');
+  if (fs.existsSync(customWebpackPath)) {
+    const customConfig = require(customWebpackPath) as { getWebpackConfig: CustomWebpackConfigurationGetter };
+    return customConfig.getWebpackConfig(baseConfig, options);


Having a meaningful error message in the console would be wise if the exported thing is not a function.

Added error if export is not a function.

sebimarkgraf · 2019-12-10T08:27:52Z

Added a example in the README to be easier to use

stevenvachon · 2019-12-10T20:06:54Z

I think that this approach too is overly complicated. Why don't we let plugin authors create their own 100% custom config? For example, they may prefer JavaScript over TypeScript. Is there an issue with allowing them to import the toolkit's default config and let them extend it themselves?:

import defaultConfig from '@grafana/toolkit/webpack-config'; // or similar
import {mergeDeep} from 'lodash';
export default mergeDeep({}, defaultConfig, {...});

ryantxu · 2019-12-10T20:12:48Z

Why don't we let plugin authors create their own 100% custom config?

100% we do... you just don't have to use toolkit and put in your own system (webpack, grunt, rollup, whatever). The trick with toolkit is allowing people to extend only a little bit.

Here is a project with a complete webpack implementation:
https://github.com/NatelEnergy/grafana-plotly-panel

or grunt:
https://github.com/grafana/simple-json-datasource

dprokop · 2019-12-11T08:09:23Z

packages/grafana-toolkit/src/config/webpack.plugin.config.ts

+export const loadWebpackConfig: WebpackConfigurationGetter = options => {
+  const baseConfig = getBaseWebpackConfig(options);
+
+  const customWebpackPath = path.resolve(process.cwd(), 'webpack.plugin.config.ts');


Why not naming this file webpack.config.ts?

I wanted to differ it from the default naming of webpack.
If we use the default webpack.config.ts everybody expects a custom config as proposed by steven.

dprokop · 2019-12-11T08:13:10Z

@stevenvachon goal with toolkit is to have a sane common config that addresses most of the use cases. I think that with this change we introduce a "safe" way of extending the basic config, without the need of creating a complete custom one. It does not differ much from what you are suggesting with merging the configs, but it has advantage of the config being typed(with WebpackConfigurationGetter) so working with it could be easier.

sebimarkgraf · 2019-12-11T08:20:58Z

@stevenvachon This approach would require the config to be more static. Currently the config depends on the given options for production and watching.

dprokop · 2019-12-12T07:50:15Z

@sebimarkgraf this seems close to mergeable. I would love to see a test verifying that it actually works. You can take some inspiration from the tests and mocks that are implemented in toolkit's config directory.

Also - did you have a chance to test it with the actual custom config?

sebimarkgraf · 2019-12-12T07:54:42Z

@dprokop I tested it to add another plugin for my own config. I will create a test and update the PR then.

sebimarkgraf · 2019-12-12T15:24:37Z

@dprokop Updated with test cases.
If you don't have any more wishes, it is ready to be merged from my side.

packages/grafana-toolkit/src/config/webpack.plugin.config.ts

stevenvachon · 2019-12-12T18:40:40Z

packages/grafana-toolkit/src/config/webpack.plugin.config.ts

+export type CustomWebpackConfigurationGetter = (
+  originalConfig: webpack.Configuration,
+  options: WebpackConfigurationOptions
+) => webpack.Configuration;

 export const findModuleFiles = (base: string, files?: string[], result?: string[]) => {
  files = files || fs.readdirSync(base);


Is it possible to use the async version of this function instead?

This would make sense if we have any tasks after the webpack task.

This would require to have the complete loading async and therefore add a lot of noise to this PR.
If you want I can still change everything to async/await.

It's probably not critical for this PR, but async functions are a Node.js best practice.

sebimarkgraf · 2019-12-16T12:12:57Z

Added async/await as additional commit

dprokop · 2019-12-16T15:51:35Z

packages/grafana-toolkit/README.md

@@ -127,6 +127,27 @@ Currently we support following Jest configuration properties:
 - [`snapshotSerializers`](https://jest-bot.github.io/jest/docs/configuration.html#snapshotserializers-array-string)
 - [`moduleNameMapper`](https://jestjs.io/docs/en/configuration#modulenamemapper-object-string-string)

+### Can I add custom Webpack rules or plugins?


Suggested change

### Can I add custom Webpack rules or plugins?

### How can I customize Webpack rules or plugins?

dprokop · 2019-12-16T16:39:00Z

@sebimarkgraf I've had a chance to test your implementation today and it works great, thanks for this!

Some final remarks:

export = getWebpackConfig this got me confused as by default I did export const getWebpackConfiguration which made the build break. I think we should add support for named export from the custom webpack config file.
I'm still not sold on the webpack.plugin.config.ts file name. That's probably only me, but I would expect it to be webpack.config.ts as we use this convention for prettier and tsconfig (eslint soon TSLint → ESLint #21006). There are 2 opinions agains mine in the PR so let's stick with the name you have proposed.
Left some minor comment about documentation. When you fix 1. then pls also update the docs and we are good to merge.

sebimarkgraf · 2019-12-17T07:54:21Z

Your argument of aligning all configurations with the original name is quite compelling. Since stevenvachon does not seem to take a definite side, I will change the naming.

* Toolkit: Add possibility to add custom webpack config * Toolkit: Refactor webpack to utilize async-await * Toolkit: Rename config file and allow named export

sebimarkgraf force-pushed the feature/20833-override-webpack-config branch from f329cd2 to 951833d Compare December 4, 2019 15:27

ryantxu requested a review from stevenvachon December 4, 2019 15:31

sebimarkgraf force-pushed the feature/20833-override-webpack-config branch 2 times, most recently from d0ac1a2 to a98163a Compare December 4, 2019 15:38

dprokop self-requested a review December 5, 2019 08:22

sebimarkgraf force-pushed the feature/20833-override-webpack-config branch from a98163a to bd1104f Compare December 6, 2019 09:30

sebimarkgraf force-pushed the feature/20833-override-webpack-config branch from bd1104f to de30f59 Compare December 6, 2019 12:53

papagian added the pr/external This PR is from external contributor label Dec 8, 2019

dprokop reviewed Dec 9, 2019

View reviewed changes

sebimarkgraf force-pushed the feature/20833-override-webpack-config branch from de30f59 to 0bafec8 Compare December 10, 2019 08:27

sebimarkgraf changed the title ~~WIP: Toolkit: Add option to override webpack config~~ Toolkit: Add option to override webpack config Dec 10, 2019

dprokop reviewed Dec 11, 2019

View reviewed changes

sebimarkgraf force-pushed the feature/20833-override-webpack-config branch 2 times, most recently from 22debd8 to 9cee96b Compare December 12, 2019 15:22

stevenvachon reviewed Dec 12, 2019

View reviewed changes

Toolkit: Add possibility to add custom webpack config

f3b7b63

sebimarkgraf force-pushed the feature/20833-override-webpack-config branch from 9cee96b to f3b7b63 Compare December 13, 2019 09:20

Toolkit: Refactor webpack to utilize async-await

736deeb

sebimarkgraf force-pushed the feature/20833-override-webpack-config branch from 175eb3f to 736deeb Compare December 16, 2019 12:31

dprokop reviewed Dec 16, 2019

View reviewed changes

dprokop added add to changelog area/grafana/toolkit labels Dec 16, 2019

dprokop changed the title ~~Toolkit: Add option to override webpack config~~ grafana/toolkit: Add option to override webpack config Dec 16, 2019

dprokop added the area/frontend label Dec 16, 2019

dprokop added this to the 6.6 milestone Dec 16, 2019

Toolkit: Rename config file and allow named export

b1b176d

stevenvachon approved these changes Dec 18, 2019

View reviewed changes

stevenvachon merged commit f1845d8 into grafana:master Dec 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

grafana/toolkit: Add option to override webpack config #20872

grafana/toolkit: Add option to override webpack config #20872

sebimarkgraf commented Dec 4, 2019

dprokop commented Dec 5, 2019

stevenvachon commented Dec 6, 2019

sebimarkgraf commented Dec 6, 2019

dprokop commented Dec 6, 2019

sebimarkgraf commented Dec 6, 2019

dprokop commented Dec 9, 2019

dprokop Dec 9, 2019

sebimarkgraf Dec 10, 2019

sebimarkgraf commented Dec 10, 2019

stevenvachon commented Dec 10, 2019 •

edited

Loading

ryantxu commented Dec 10, 2019 •

edited

Loading

dprokop Dec 11, 2019

sebimarkgraf Dec 11, 2019

dprokop commented Dec 11, 2019

sebimarkgraf commented Dec 11, 2019

dprokop commented Dec 12, 2019

sebimarkgraf commented Dec 12, 2019

sebimarkgraf commented Dec 12, 2019

stevenvachon Dec 12, 2019

sebimarkgraf Dec 13, 2019

stevenvachon Dec 13, 2019

sebimarkgraf commented Dec 16, 2019

dprokop Dec 16, 2019

dprokop commented Dec 16, 2019

sebimarkgraf commented Dec 17, 2019

	### Can I add custom Webpack rules or plugins?
	### How can I customize Webpack rules or plugins?

grafana/toolkit: Add option to override webpack config #20872

grafana/toolkit: Add option to override webpack config #20872

Conversation

sebimarkgraf commented Dec 4, 2019

dprokop commented Dec 5, 2019

stevenvachon commented Dec 6, 2019

sebimarkgraf commented Dec 6, 2019

dprokop commented Dec 6, 2019

sebimarkgraf commented Dec 6, 2019

dprokop commented Dec 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebimarkgraf commented Dec 10, 2019

stevenvachon commented Dec 10, 2019 • edited Loading

ryantxu commented Dec 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dprokop commented Dec 11, 2019

sebimarkgraf commented Dec 11, 2019

dprokop commented Dec 12, 2019

sebimarkgraf commented Dec 12, 2019

sebimarkgraf commented Dec 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebimarkgraf commented Dec 16, 2019

Choose a reason for hiding this comment

dprokop commented Dec 16, 2019

sebimarkgraf commented Dec 17, 2019

stevenvachon commented Dec 10, 2019 •

edited

Loading

ryantxu commented Dec 10, 2019 •

edited

Loading