Small Plugin Cleanup #206
Small Plugin Cleanup #206
Conversation
Signed-off-by: Luke Tillman <LukeTillman@users.noreply.github.com>
Signed-off-by: Luke Tillman <LukeTillman@users.noreply.github.com>
Signed-off-by: Luke Tillman <LukeTillman@users.noreply.github.com>
Signed-off-by: Luke Tillman <LukeTillman@users.noreply.github.com>
Signed-off-by: Luke Tillman <LukeTillman@users.noreply.github.com>
Signed-off-by: Luke Tillman <LukeTillman@users.noreply.github.com>
Signed-off-by: Luke Tillman <LukeTillman@users.noreply.github.com>
Signed-off-by: Luke Tillman <LukeTillman@users.noreply.github.com>
Signed-off-by: Luke Tillman <LukeTillman@users.noreply.github.com>
| @@ -12,6 +12,20 @@ | |||
| } | |||
| } | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
Just adding a browser data source to the sample data here...
| @@ -21,6 +21,7 @@ import { | |||
| PluginType, | |||
| AnyPluginImplementation, | |||
| JsonObject, | |||
| ALL_PLUGIN_TYPES, | |||
There was a problem hiding this comment.
This change to have a runtime list of all plugin types simplifies some of the registry code below so that now there are fewer places to go make changes whenever we add a new plugin type.
| @@ -20,7 +20,7 @@ import App from './App'; | |||
| import { createTheme } from './styles/theme'; | |||
| import { SnackbarProvider } from './context/SnackbarProvider'; | |||
|
|
|||
| const queryClient = new QueryClient(); | |||
| const queryClient = new QueryClient({ defaultOptions: { queries: { refetchOnWindowFocus: false } } }); | |||
There was a problem hiding this comment.
This just turns off the default react-query behavior that was causing PromQL queries, etc. to be refetched anytime you focus the browser window.
There was a problem hiding this comment.
cool ! That was simple at then. Nice catch !
Do you know what this behavior is the one by default ? It's a bit odd to have it by default, but probably I miss some context
There was a problem hiding this comment.
Yeah, I felt like it was a weird choice to have by default too, so don't think you're missing anything. The docs have a bit of an explanation of some of the defaults:
| @@ -35,6 +35,7 @@ export const commonConfig: Configuration = { | |||
| new ForkTsCheckerWebpackPlugin({ | |||
| typescript: { | |||
| configFile: path.resolve(__dirname, './tsconfig.json'), | |||
| build: true, // Since we use project references... | |||
There was a problem hiding this comment.
This should fix the error that we see sometimes locally where TypeScript complains that "XXX has not been built from source file YYY". I also tried to clean-up the tsconfig options a bit. So far seems to have worked 🤞
| @@ -60,6 +60,7 @@ const devConfig: Configuration = { | |||
| port: parseInt(process.env.PORT ?? '3000'), | |||
| open: true, | |||
| https: getHttpsConfig(), | |||
| http2: process.env.HTTP2 === 'true', | |||
There was a problem hiding this comment.
Allows enabling HTTP2 for the webpack dev server (like I mentioned in chat).
| @@ -11,8 +11,7 @@ | |||
| "isolatedModules": true, | |||
| "noUncheckedIndexedAccess": true, | |||
| "composite": true, | |||
| "paths": { | |||
| "@perses-ui/*": ["./*/src"] | |||
There was a problem hiding this comment.
the unit tests are failing. I'm wondering if it's related to this change.
I'm surprised we don't need it because when I worked around this topic and the possibility to run npm start, this part was the key to make it works.
There was a problem hiding this comment.
Oh man, this sent me down a rabbit hole. 😂
OK, so you're right, this change is why the tests are failing to compile. But with Typescript project references, this setting isn't actually supposed to be necessary. With project references, the references config in tsconfig.json points to the other projects that a given package depends on. Then when you run tsc --build (you have to use --build to get project references to compile correctly), the Typescript compiler takes into account the references config and builds those projects first. This is also why I made the change to add build: true to our webpack config (so that it uses project references behavior).
The problem is that jest and ts-jest in particular doesn't currently deal with project references. Since it's a jest transformer that runs "on-demand" for individual files, it doesn't actually have a way to use the --build flag that would take care of building project references first. There's an open issue in ts-jest currently where they're discussing how to deal with project references and has some workarounds.
I'm going to think about this a bit more, but I think for now, I may just use one of the workarounds (e.g. moduleNameMapper) suggested in that issue or just bring back this config for the time being...
There was a problem hiding this comment.
I ended up bringing this config back in a tsconfig.test.json for running unit tests. We'll see what ts-jest ends up ultimately doing to better support project references.
Signed-off-by: Luke Tillman <LukeTillman@users.noreply.github.com>
|
Sorry about the CI noise if anyone else is getting notified, trying out a few things. |
Signed-off-by: Luke Tillman <LukeTillman@users.noreply.github.com>
Signed-off-by: Luke Tillman <LukeTillman@users.noreply.github.com>
This is a small clean-up to the plugin types to remove the
Kindtype parameter that's being used everywhere. It's not actually adding anything useful sinceKindwill always just be astringat runtime when dealing with it in the Perses app code and so it was making those types a lot more verbose and confusing than they actually needed to be.I also cleaned up a few other small things and I'll try and leave comments inline to describe what's up. I think I'll have a slightly larger refactoring of those plugin types coming soon that should really clean things up and make them less confusing and verbose.