refactor: move translation, connection, query to core #729

ktmud · 2020-08-10T19:37:17Z

🏠 Internal

Follow up of #728 .

Started with merging connection and query into core, because they are closely related. Ended up with moving translation as well because it was used in test/setup.js.

vercel · 2020-08-10T19:37:21Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/superset/superset-ui/o09ba5wtp
✅ Preview: https://superset-ui-git-fork-ktmud-monopackage-move-connection.superset.vercel.app

ktmud · 2020-08-10T19:58:45Z

packages/superset-ui-chart-controls/src/types.ts

@@ -18,7 +18,7 @@
 * under the License.
 */
 import React, { ReactNode, ReactText, ReactElement } from 'react';
-import { QueryFormData } from '@superset-ui/query';
+import { QueryFormData } from '@superset-ui/core/lib/query';


test files uses src/, cross-package references use lib/.

ktmud · 2020-08-10T19:59:49Z

packages/superset-ui-core/src/connection/callApi/rejectAfterTimeout.ts

@@ -3,6 +3,7 @@ export default function rejectAfterTimeout<T>(timeout: number) {
  return new Promise<T>((resolve, reject) => {
    setTimeout(
      () =>
+        // eslint-disable-next-line prefer-promise-reject-errors


Previously disabled in packages/superset-ui/connection/.eslintrc.

docs/debugging.md

kristw · 2020-08-10T21:06:34Z

Now that they are combined into one package you could export everything that used to be a top-level export from query and connection as top-level export from @superset-ui/core so there is no need for the deep import (/lib/xxx).

export * from './query`
export * from './connection`

suddjian · 2020-08-10T21:14:03Z

...y-plugins/hold-potentially-deprecate/superset-ui-legacy-plugin-chart-word-cloud/package.json

@@ -35,6 +35,6 @@
  "peerDependencies": {
    "@superset-ui/chart": "^0.14.0",
    "@superset-ui/color": "^0.14.0",
-    "@superset-ui/translation": "^0.14.0"
+    "@superset-ui/core": "^0.15.0"


Is the intention to release 0.15 once this is merged?

ktmud · 2020-08-10T21:16:35Z

Now that they are combined into one package you could export everything that used to be a top-level export from query and connection as top-level export from @superset-ui/core so there is no need for the deep import (/lib/xxx).
export * from './query`
export * from './connection`

I'd rather fix this in another PR since there may be export conflicts.

kristw · 2020-08-10T21:19:19Z

I'd rather fix this in another PR since there may be export conflicts.

I don't think there is. The @superset-ui/superset-ui package already did this and there was no naming conflict. (vscode will also warn if you did multiple export * and there are naming conflicts) Then you can remove all the import lib/ changes too.

kristw · 2020-08-10T21:59:53Z

packages/superset-ui-chart-composition/test/legend/WithLegend.test.tsx

@@ -5,7 +5,7 @@ import { mount, shallow } from 'enzyme';
 jest.mock('resize-observer-polyfill');
 // @ts-ignore
 import { triggerResizeObserver } from 'resize-observer-polyfill';
-import { promiseTimeout } from '@superset-ui/core';
+import { promiseTimeout } from '@superset-ui/core/src';


Does this need src?
It did not need this previously.

#729 (comment)

But this is not the test for that function or package. @superset-ui/core is only a dependency.

This test the WithLegend component and only use promiseTimeout for facilitating the test, not testing promiseTimeout itself.

This makes sense. I removed src for other packages.

kristw · 2020-08-10T22:00:17Z

packages/superset-ui-chart/README.md

@@ -44,7 +44,7 @@ requests from domains outside your `Apache Superset` instance:
   configuration in the `@superset-ui` storybook.

   ```javascript
-   import { SupersetClient } from '@superset-ui/connection';
+   import { SupersetClient } from '@superset-ui/core/src';


remove /src

kristw · 2020-08-10T22:00:51Z

packages/superset-ui-chart/test/components/MockChartPlugins.tsx

@@ -1,6 +1,6 @@
 /* eslint-disable max-classes-per-file */
 import React from 'react';
-import { QueryFormData } from '@superset-ui/query';
+import { QueryFormData } from '@superset-ui/core/src';


ditto. /src probably not needed?

kristw · 2020-08-10T22:02:58Z

test/setup.ts

@@ -1,4 +1,4 @@
-import { configure } from '@superset-ui/translation';
+import { configure } from '@superset-ui/core/src/translation';


remove /src/translation

import { configure } from '@superset-ui' feels unclear to me. It's not clear what does it configure. Might change it to use the same pattern as connection/SupersetClient.

But maybe in another PR.

kristw · 2020-08-10T22:04:26Z

Thank you for doing the massive work

ktmud · 2020-08-10T22:54:30Z

package.json

@@ -24,7 +24,7 @@
    "prettier": "nimbus prettier",
    "test": "yarn jest",
    "test:watch": "yarn lint:fix && yarn jest --watch",
-    "type": "nimbus typescript --build --reference-workspaces",
+    "type": "yarn build \"*\" --type-only",


Very nice change here. See build.js for details.

ktmud · 2020-08-11T19:57:15Z

temporary-plugins/superset-ui-plugins-demo/storybook/stories/index.js

-import { configure } from '@superset-ui/translation';
+import categoricalD3 from '@superset-ui/color/lib/colorSchemes/categorical/d3';
+import sequentialCommon from '@superset-ui/color/lib/colorSchemes/sequential/common';
+import sequentialD3 from '@superset-ui/color/lib/colorSchemes/sequential/d3';


We are not generating esm files any more, since the content is identical to lib (checked multiple random files).

I dig a bit deeper and found that last time I modified babel config there was a mistake so both esm building and commonjs (lib) ended up pointing to same babel config so the esm output now is not correctly built.

We should fix this (not in this PR, but just don't drop esm) so the esm output will be different from lib and using import/export instead of exports.xxx. The esm output should be more preferred for webpack than lib.

So, what we've learned is that in order to take advantage of tree shaking, you must...

Use ES2015 module syntax (i.e. import and export).

Ensure no compilers transform your ES2015 module syntax into CommonJS modules (this is the default behavior > of the popular Babel preset @babel/preset-env - see the documentation for more details).

Add a "sideEffects" property to your project's package.json file.

from webpack doc https://webpack.js.org/guides/tree-shaking/#clarifying-tree-shaking-and-sideeffects

Example of esm vs lib output

Just pushed a fix in my branch: 18b05b1

ktmud · 2020-08-11T20:00:34Z

jest.config.js

+      "testEnvironment": "node"
+    }
+  ]
+};


Configs still generated by nimbus, we just don't re-generate it every time CI needs to run.

Missing EOF new line is from nimbus, decided not to reformat since this file may be overridden again anyway.

ktmud · 2020-08-11T20:01:37Z

@kristw @suddjian I think this is ready for merge. A major update from yesterday is moving away from nimbus test/build runners and the updated build script. Please take another look if you have time.

suddjian

LGTM, thank you for tackling this!

…t#729)

1. move translation, connection, query to core (apache-superset#729) 2. update encodable to remove formatter inter-dependency (apache-superset#744) 3. move number-format and time-format to core (apache-superset#730)

1. move translation, connection, query to core (#729) 2. update encodable to remove formatter inter-dependency (#744) 3. move number-format and time-format to core (#730)

1. move translation, connection, query to core (#729) 2. update encodable to remove formatter inter-dependency (#744) 3. move number-format and time-format to core (#730) 4: move superset-ui/dimension to core (#732)

1. move translation, connection, query to core (#729) 2. update encodable to remove formatter inter-dependency (#744) 3. move number-format and time-format to core (#730) 4: move superset-ui/dimension to core (#732) 5: move superset-ui/color to core (#755)

1. move translation, connection, query to core (#729) 2. update encodable to remove formatter inter-dependency (#744) 3. move number-format and time-format to core (#730) 4. move superset-ui/dimension to core (#732) 5. move superset-ui/color to core (#755) 6. move superset-ui/style to core (#756) 7. move superset-ui/validator to core (#757)

1. move translation, connection, query to core (#729) 2. update encodable to remove formatter inter-dependency (#744) 3. move number-format and time-format to core (#730) 4. move superset-ui/dimension to core (#732) 5. move superset-ui/color to core (#755) 6. move superset-ui/style to core (#756) 7. move superset-ui/validator to core (#757) 8: move superset-ui/chart-composition to core (#759)

1. move translation, connection, query to core (#729) 2. update encodable to remove formatter inter-dependency (#744) 3. move number-format and time-format to core (#730) 4. move superset-ui/dimension to core (#732) 5. move superset-ui/color to core (#755) 6. move superset-ui/style to core (#756) 7. move superset-ui/validator to core (#757) 8: move superset-ui/chart-composition to core (#759) 9: move superset-ui/chart to core (#760)

1. move translation, connection, query to core (#729) 2. update encodable to remove formatter inter-dependency (#744) 3. move number-format and time-format to core (#730) 4. move superset-ui/dimension to core (#732) 5. move superset-ui/color to core (#755) 6. move superset-ui/style to core (#756) 7. move superset-ui/validator to core (#757) 8. move superset-ui/chart-composition to core (#759) 9. move superset-ui/chart to core (#760)

ktmud requested a review from a team as a code owner August 10, 2020 19:37

vercel bot had a problem deploying to Preview August 10, 2020 19:37 Failure

vercel bot deployed to Preview August 10, 2020 19:40 View deployment

vercel bot temporarily deployed to Preview August 10, 2020 19:54 Inactive

vercel bot had a problem deploying to Preview August 10, 2020 19:55 Failure

ktmud commented Aug 10, 2020

View reviewed changes

kristw reviewed Aug 10, 2020

View reviewed changes

docs/debugging.md Outdated Show resolved Hide resolved

vercel bot temporarily deployed to Preview August 10, 2020 20:52 Inactive

vercel bot had a problem deploying to Preview August 10, 2020 20:53 Failure

vercel bot deployed to Preview August 10, 2020 21:04 View deployment

suddjian reviewed Aug 10, 2020

View reviewed changes

vercel bot deployed to Preview August 10, 2020 21:34 View deployment

vercel bot deployed to Preview August 10, 2020 21:47 View deployment

pull-request-size bot added the size/XXL label Aug 10, 2020

vercel bot temporarily deployed to Preview August 10, 2020 21:55 Inactive

ktmud force-pushed the monopackage-move-connection branch from 7899b08 to b93631b Compare August 10, 2020 21:57

pull-request-size bot added size/XL and removed size/XXL labels Aug 10, 2020

vercel bot had a problem deploying to Preview August 10, 2020 21:57 Failure

kristw reviewed Aug 10, 2020

View reviewed changes

pull-request-size bot removed the size/XL label Aug 10, 2020

vercel bot had a problem deploying to Preview August 10, 2020 22:51 Failure

pull-request-size bot added the size/XXL label Aug 10, 2020

ktmud commented Aug 10, 2020

View reviewed changes

ktmud commented Aug 11, 2020

View reviewed changes

ktmud mentioned this pull request Aug 11, 2020

refactor: move superset-ui/dimension to core #732

Merged

suddjian approved these changes Aug 11, 2020

View reviewed changes

Build esm modules

18b05b1

vercel bot deployed to Preview August 11, 2020 22:46 View deployment

kristw approved these changes Aug 11, 2020

View reviewed changes

Update logging

1e17edb

vercel bot deployed to Preview August 11, 2020 23:03 View deployment

ktmud merged commit 618235f into apache-superset:monopackage Aug 11, 2020

ktmud added a commit to ktmud/superset-ui that referenced this pull request Aug 26, 2020

refactor: move translation, connection, query to core (apache-superse…

6d04eb5

…t#729)

ktmud mentioned this pull request Aug 26, 2020

refactor: move superset-ui/color to core #755

Merged

ktmud added a commit that referenced this pull request Aug 26, 2020

refactor: merge core superset-ui packages

9078a87

1. move translation, connection, query to core (#729) 2. update encodable to remove formatter inter-dependency (#744) 3. move number-format and time-format to core (#730)

This was referenced Aug 26, 2020

refactor: move superset-ui/style to core #756

Merged

refactor: move superset-ui/validator to core #757

Merged

This was referenced Aug 27, 2020

refactor: move superset-ui/chart-composition to core #759

Merged

refactor: move chart to core #760

Merged

ktmud mentioned this pull request Sep 3, 2020

refactor: merge core superset-ui packages #768

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move translation, connection, query to core #729

refactor: move translation, connection, query to core #729

ktmud commented Aug 10, 2020

vercel bot commented Aug 10, 2020 •

edited

Loading

ktmud Aug 10, 2020

ktmud Aug 10, 2020

kristw commented Aug 10, 2020 •

edited

Loading

suddjian Aug 10, 2020

ktmud Aug 10, 2020

ktmud commented Aug 10, 2020

kristw commented Aug 10, 2020

kristw Aug 10, 2020

suddjian Aug 10, 2020

kristw Aug 10, 2020

kristw Aug 10, 2020

ktmud Aug 11, 2020

kristw Aug 10, 2020

kristw Aug 10, 2020

kristw Aug 10, 2020

ktmud Aug 10, 2020

ktmud Aug 10, 2020

kristw commented Aug 10, 2020

ktmud Aug 10, 2020

ktmud Aug 11, 2020

kristw Aug 11, 2020 •

edited

Loading

ktmud Aug 11, 2020

ktmud Aug 11, 2020

ktmud commented Aug 11, 2020

suddjian left a comment

		@@ -1,4 +1,4 @@
		import { configure } from '@superset-ui/translation';
		import { configure } from '@superset-ui/core/src/translation';

refactor: move translation, connection, query to core #729

refactor: move translation, connection, query to core #729

Conversation

ktmud commented Aug 10, 2020

vercel bot commented Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristw commented Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud commented Aug 10, 2020

kristw commented Aug 10, 2020

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristw commented Aug 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristw Aug 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud commented Aug 11, 2020

suddjian left a comment

Choose a reason for hiding this comment

vercel bot commented Aug 10, 2020 •

edited

Loading

kristw commented Aug 10, 2020 •

edited

Loading

kristw Aug 11, 2020 •

edited

Loading