Create Plugin: Extend the playwright config file

[oshirohugo]

What this PR does / why we need it:

This PR move the existing playwright config to ./config, this will make updating the default playwright config easier.

Which issue(s) this PR fixes:

Closes #1366

Special notes for your reviewer:

The way the docs are written right now is still applicable to the new playwright config that extends the one from ./config therefore they were not modified.

📦 Published PR as canary version: Canary Versions

✨ Test out this PR locally via:

npm install @grafana/create-plugin@5.27.0-canary.1495.17868716647.0
npm install @grafana/plugin-meta-extractor@0.10.0-canary.1495.17868716647.0
# or 
yarn add @grafana/create-plugin@5.27.0-canary.1495.17868716647.0
yarn add @grafana/plugin-meta-extractor@0.10.0-canary.1495.17868716647.0

[github-actions]

Hello! 👋 This repository uses Auto for releasing packages using PR labels.

✨ This PR can be merged and will trigger a new minor release.
NOTE: When merging a PR with the release label please avoid merging another PR. For further information see here.

[sunker]

I'm a bit sceptical to this. This will make it a little harder to overview and edit the Playwright config for plugin developers. And to my knowledge, Playwright don't offer any abstraction to merge configs (the way for example Webpack does with [https://www.npmjs.com/package/webpack-merge]).

Now that we're soon to release the new create-plugin update cmd that allows us to safely manipulate files outside the .config dir, is this really necessary? cc @jackw who initially raised this issue.

[mckn]

I'm a bit sceptical to this. This will make it a little harder to overview and edit the Playwright config for plugin developers. And to my knowledge, Playwright don't offer any abstraction to merge configs (the way for example Webpack does with [https://www.npmjs.com/package/webpack-merge]).

Now that we're soon to release the new create-plugin update cmd that allows us to safely manipulate files outside the .config dir, is this really necessary? cc @jackw who initially raised this issue.

Merging the configs isn't a problem since playwright expects the default object exposed from that file to be the config. So you can use what ever merge util you want to merge multiple configs.

But I agree with your thoughts regarding the migrations. However, it might not be easy to migrate a file that is changed by the user. So moving the base to .config folder might be a good idea regardless. WDYT?

[jackw]

I'm a bit sceptical to this. This will make it a little harder to overview and edit the Playwright config for plugin developers. And to my knowledge, Playwright don't offer any abstraction to merge configs (the way for example Webpack does with [https://www.npmjs.com/package/webpack-merge]).

As others have also pointed out we can rely on spreading these configs but I believe Playwright does have some concept of merging configs via defineConfig which can take multiple configs. e.g. defineConfig(defaultConfig, {});.

However, the playwright team decided that the projects property should override which would result in needing to do:

export default defineConfig(defaultConfig, {
  projects: [
    ...defaultConfig.projects!,
    {
      name: 'firefox',
      use: { ...devices['Desktop Firefox'], storageState: 'playwright/.auth/admin.json' },
      dependencies: ['auth'],
    },
  ],
});

Whilst it was originally designed for webpack, webpack-merge isn't webpack specific. You can use it for any config including playwrights:

import { merge } from 'webpack-merge';
import defaultConfig from './.config/playwright.config';

const config = merge(defaultConfig, {
  projects: [
    {
      name: 'firefox',
      use: { ...devices['Desktop Firefox'], storageState: 'playwright/.auth/admin.json' },
      dependencies: ['auth'],
    },
  ],
});

export default defineConfig(config);

Now that we're soon to release the new create-plugin update cmd that allows us to safely manipulate files outside the .config dir, is this really necessary?

Within create-plugin, we have the concept of "put default configs in .config directory and extend in files in root of project" and I opened the issue to align our playwright config with this concept. I also agree with @mckn that even with migrations it's much easier to update a file in the .config directory as we consider them ours and document in the files and in our docs website that touching them is a bad idea.

[jackw]

Suggested change

	import type { PluginOptions } from '@grafana/plugin-e2e';
	/*
	* ⚠️⚠️⚠️ THIS FILE WAS SCAFFOLDED BY `@grafana/create-plugin`. DO NOT EDIT THIS FILE DIRECTLY. ⚠️⚠️⚠️
	*
	* In order to extend the configuration follow the steps in
	* https://grafana.com/developers/plugin-tools/get-started/set-up-development-environment#extend-the-playwright-config
	*/
	import type { PluginOptions } from '@grafana/plugin-e2e';

If we decide to do this I think we should update the docs site in this PR too with instructions.

[jackw]

Not sure this is necessary. I think you can export the defineConfig<PluginOptions>({}) in this file then use defineConfig again in the root.

[oshirohugo]

@jackw @mckn could you please check this one again?

[leventebalogh]

Even though I understand @sunker's points, I am also leaning towards having all "basic" configuration files under the .config/ folder (mostly for the reason that was already brought up before, so it's easier for us to decide programatically what was added by the user and what's the default config we provided).

The changes look good to me, but maybe better to also wait for @jackw 👍

[jackw]

LGTM ! 🚀

See my previous comment about overrides to project property with multiple defineConfig calls. Might be useful to surface this in a docs update?

[oshirohugo]

@jackw I've update the docs to reflect the new config extensions and I also added a section to our docs trying to cover the issues you mentioned, please take a look if they are ok :D

[oshirohugo]

@jackw is this PR still relevant? If yes, I'll create another PR in a fresher branch.

[github-actions]

This pull request has been automatically marked as stale because it has not had activity in the last 90 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[github-actions]

This pull request has been automatically closed because it has not had activity in the last 2 weeks. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!