Update rollup bundling config #33

kevinwcyu · 2022-12-09T19:36:49Z

Fixes: #30

Update rollup config to take advantage of treeshaking and esm builds.

As an example, see the config for @grafana/ui which is supposed to be an up-to-date config for bundling libraries.

Quick manual tests with Cloudwatch, Athena, Redshift, Timestream, and X-Ray by checking the config editor and the query editor and everything still works.

kevinwcyu · 2022-12-09T19:41:03Z

src/sql/ConfigEditor/ConfigSelect.tsx

@@ -70,7 +70,6 @@ export function ConfigSelect(props: ConfigSelectProps) {
      allowCustomValue={props.allowCustomValue}
      autoFocus={props.autoFocus}
      backspaceRemovesValue={props.backspaceRemovesValue}
-      className={props.className}


I removed passing the className prop down as the current behaviour is already overriding the className (see commonProps on line 43).

Should we still be passing 'width-30' as the className?

It's still being passed by commonProps.

iwysiu

The comments are less critique and more questions. I hadn't heard of some of these concepts before, so someone else should probably also review it

iwysiu · 2022-12-09T21:03:30Z

src/sql/ResourceSelector.test.tsx

@@ -24,6 +24,10 @@ describe('ResourceSelector', () => {
    render(<ResourceSelector {...props} default="foo" value={defaultKey} fetch={fetch} onChange={onChange} />);
    expect(screen.queryByText('default (foo)')).toBeInTheDocument();

+    // TODO ResourceSelectorProps should be updated to make `label` required


Is this todo for in this pr or a reminder for a follow up issue?

Reminder for a follow up, could potentially be done in this PR though, just have to go through the places where this component is used and make sure it can be changed to a required prop.

Actually, going to leave the prop as optional to match the InlineField component's label prop. I'll remove this TODO comment and close the other PR.

iwysiu · 2022-12-09T21:14:20Z

src/sql/ConfigEditor/ConfigSelect.tsx

@@ -70,7 +70,6 @@ export function ConfigSelect(props: ConfigSelectProps) {
      allowCustomValue={props.allowCustomValue}
      autoFocus={props.autoFocus}
      backspaceRemovesValue={props.backspaceRemovesValue}
-      className={props.className}


Should we still be passing 'width-30' as the className?

iwysiu · 2022-12-09T21:15:34Z

src/index.ts

@@ -1,8 +1,8 @@
-export { ConnectionConfig, ConnectionConfigProps } from './ConnectionConfig';
+export { ConnectionConfig, type ConnectionConfigProps } from './ConnectionConfig';


What does adding type here do?

In Typescript, types are erased because js doesn't actually have the same runtime concept of types like ts. But when code is being transpiled, this export statement is actually trying to find something exported from the ConnectionConfig module, but since it's erased, it doesn't actually find anything. Adding type here indicates that it's just importing a type and there's nothing actually exported.

sunker

Nice! And thanks for switching to Rollup's watcher. It's so much faster now.

How come we don't generate one index file for development and one for production anymore?

sarahzinger

I'm not a rollup/bundling expert but this looks cool to me! Yay tree shaking!

There are a couple of neat new(ish?) things here that I didn't know about like importing just types or this React.DependencyList thing, might be cool to do a little lightning talk or something at a frontend weekly to share in case other people are interested :) I bet I'm not the only one new to it!

sarahzinger · 2022-12-12T16:45:43Z

package.json

    "dist"
  ],
  "repository": "github:grafana/grafana-aws-sdk-react",
-  "author": "Grafana Labs <[email protected]> (https://grafanap.com)",


sarahzinger · 2022-12-12T16:49:27Z

src/sql/ConfigEditor/ConfigSelect.tsx

@@ -1,4 +1,4 @@
-import React from 'react';
+import React, { DependencyList } from 'react';


👀 I don't think I've heard of this one!

kevinwcyu · 2022-12-12T21:22:09Z

How come we don't generate one index file for development and one for production anymore?

@sunker I was following the reasoning from the @grafana/ui config

We currently bundle index.development.js and index.production.js files for all packages. This PR removes them in favour of a single build (packaged as CJS and ESM) without minification and includes sourcemaps. Reasoning for this change is that if a consumer wants to use this package their app build tooling can minify the package. Likewise for plugin developers these packages are only used in test environments making a minified production build a bit redundant IMHO.

— grafana/grafana#51517 (comment)

sunker

@sunker I was following the reasoning from the @grafana/ui config

Makes sense! Thanks

kevinwcyu requested a review from a team as a code owner December 9, 2022 19:36

kevinwcyu requested review from iwysiu and sarahzinger and removed request for a team December 9, 2022 19:36

kevinwcyu commented Dec 9, 2022

View reviewed changes

iwysiu reviewed Dec 9, 2022

View reviewed changes

sunker reviewed Dec 12, 2022

View reviewed changes

sarahzinger approved these changes Dec 12, 2022

View reviewed changes

sunker approved these changes Dec 13, 2022

View reviewed changes

Update rollup config

87715d7

idastambuk approved these changes Dec 14, 2022

View reviewed changes

kevinwcyu force-pushed the kevinwcyu/update-rollup-config branch from 9780eb1 to 87715d7 Compare December 14, 2022 19:37

kevinwcyu added 2 commits December 14, 2022 11:47

remove todo comment

1f5a045

bump version

e7cb8aa

kevinwcyu merged commit 9d2b0c1 into main Dec 14, 2022

kevinwcyu deleted the kevinwcyu/update-rollup-config branch December 14, 2022 19:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update rollup bundling config #33

Update rollup bundling config #33

kevinwcyu commented Dec 9, 2022

kevinwcyu Dec 9, 2022

iwysiu Dec 9, 2022

kevinwcyu Dec 9, 2022

iwysiu left a comment

iwysiu Dec 9, 2022

kevinwcyu Dec 9, 2022

kevinwcyu Dec 14, 2022 •

edited

Loading

iwysiu Dec 9, 2022

iwysiu Dec 9, 2022

kevinwcyu Dec 9, 2022

sunker left a comment

sarahzinger left a comment

sarahzinger Dec 12, 2022

sarahzinger Dec 12, 2022

idastambuk Dec 14, 2022

kevinwcyu commented Dec 12, 2022

sunker left a comment

		@@ -1,8 +1,8 @@
		export { ConnectionConfig, ConnectionConfigProps } from './ConnectionConfig';
		export { ConnectionConfig, type ConnectionConfigProps } from './ConnectionConfig';

		@@ -1,4 +1,4 @@
		import React from 'react';
		import React, { DependencyList } from 'react';

Update rollup bundling config #33

Update rollup bundling config #33

Conversation

kevinwcyu commented Dec 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwysiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinwcyu Dec 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunker left a comment

Choose a reason for hiding this comment

sarahzinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinwcyu commented Dec 12, 2022

sunker left a comment

Choose a reason for hiding this comment

kevinwcyu Dec 14, 2022 •

edited

Loading