Separate visualizations into their own package #4837

gabrieldutra · 2020-04-25T22:31:14Z

What type of PR is this? (check all applicable)

Other

Description

Separate visualizations into their own package

To-do list:

Choose components to be shared instead of copied
Remove p-r-5 from redash-visualizations/src/visualizations/table/utils.js
Remove visualizations dependencies from main package.json
Adapt main build scripts
- Netlify
- Circleci (50%)
- Dev env
Fix jest tests

Related Tickets & Documents

--

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

--

arikfr

See comments.

Also the folder in the repo, how about we name it viz-lib?

arikfr · 2020-04-26T11:53:08Z

redash-visualizations/package.json

@@ -0,0 +1,77 @@
+{
+  "name": "redash-visualizations",


Let's name it @redash/viz. I've added you to the redash org on npm, so you should be able to eventually push it there.

arikfr · 2020-04-26T11:54:03Z

redash-visualizations/package.json

@@ -0,0 +1,77 @@
+{
+  "name": "redash-visualizations",
+  "version": "1.0.0",


Let's start with version 0.1.0 and switch to 1.0 when the API is more stable.

gabrieldutra · 2020-04-28T13:33:02Z

viz-lib/src/visualizations/chart/Editor/ColorsSettings.test.js

@@ -70,7 +70,7 @@ describe("Visualizations -> Chart -> Editor -> Colors Settings", () => {
        .simulate("click");
    });

-    test("Sets custom color scheme", async done => {
+    test("Sets custom color scheme", done => {


Was this async important for something I'm missing? It was causing this test to fail removing it didn't cause any harm 🤷‍♂️
(Probably some babel setting missing for it to work, but it was way easier to just remove it)

I think async is not needed here, so it's ok to remove. Return value is always ignored, and done is used for async tests

gabrieldutra · 2020-04-28T19:43:20Z

Dockerfile

@@ -2,7 +2,8 @@ FROM node:12 as frontend-builder

 WORKDIR /frontend
 COPY package.json package-lock.json /frontend/
-RUN npm ci
+COPY viz-lib /frontend/viz-lib
+RUN npm ci --unsafe-perm


This seems to be required to run the preinstall script (which installs viz-lib deps and build it) when we run the command as root user. An alternative is to switch the user to the redash one in this scope. I tried to switch to it for all commands, but didn't seem that quick. Another option is to switch to the user only while running this command.

but didn't seem that quick

in terms of effort required to make the switch?

Regardless, --unsafe-perm should be safe in the context of running inside a Docker container build...

in terms of effort required to make the switch?

In terms of changes in that file, mostly needs to update chown/chmod permissions for that folder. As the --unsafe-perm seemed simpler, I wanted to show it as an option, but needed to confirm regarding safety. I actually didn't sit down and actually tried to change thos yesterday, but I did today and I could get it to work like this:

FROM node:12 as frontend-builder RUN useradd --create-home redash WORKDIR /frontend COPY package.json package-lock.json webpack.config.js /frontend/ COPY viz-lib /frontend/viz-lib COPY client /frontend/client RUN chown -R redash /frontend USER redash RUN npm ci RUN npm run build

(notice I changed the COPY order to run only one chown, not sure if there was a reason to have it after the npm ci command though)

Anyway, LMK which option you prefer.

(notice I changed the COPY order to run only one chown, not sure if there was a reason to have it after the npm ci command though)

The reason for the current order is to make sure that we don't have to reinstall if package.json/package-lock.json didn't change, as they change less frequently than the codebase.

Let's go with the unsafe option for now.

daniellangnet · 2020-04-29T04:54:50Z

I'm intrigued by where this is going! 👍
Just curious what the bigger picture is here so that I can decide when it makes sense for me to update my KPI visualization branch?

gabrieldutra · 2020-04-29T13:23:45Z

Hi @daniellangnet, the idea here is to have visualizations to stand on their own (not to be dependent on any other part of Redash), so they can be turned into a separate package. All the bigger changes for that to be possible were already delivered in past PRs, so this one is mostly a setup + move the visualizations folder. In any case, this is the last one that will have "more significant" changes on the codebase (although it just moves files and it should be an easy merge). The goal is to make it transparent to development flows, so ideally it shouldn't affect your work. Thanks for contributing and I'll give a quick check on your PR anyways. 🙂

gabrieldutra · 2020-04-29T15:11:06Z

This PR changes in Brief for ones that will review it:

Create viz-lib package.json and set it up with webpack to generate UMD bundled redash-visualizations.js and babel to just transpile it for more specific usage (in Redash for ex). I wanted to offer a transpiled version with css files instead of less too, but that seemed too much hassle;
Copied all visualizations folder to the viz-lib one;
Dev environment runs babel:watch on the lib in the background;
Added a build step for visualizations as a preinstall script (seemed the simplest way to make it transparent, not to require extra commands on the flow we have today). Future alternatives include maybe move it to our own bin scripts.
Separated visualizations Jest tests. Those were easy because they are unit tests, for the Cypress ones it's a bit more complicated since they are Redash tests (and it does make sense to have Redash e2e tests for it), but eventually we can move the more "unitary" of them to be tested through Storybook or sth;
Deleted duplicated files, a few components are now imported from the viz lib, though there are some duplicated that I didn't find to make sense to locate on the lib (e.g: sanitize service, which is basically our settings over DOMPurify). LMK if you think other should be in the viz lib too.

arikfr · 2020-05-05T06:58:29Z

Should we merge this now?

arikfr · 2020-05-06T07:50:14Z

🚀

gabrieldutra added 2 commits April 25, 2020 15:59

Add visualizations project settings

f037137

Move visualizations to redash-visualizations

2f19611

restyled-io bot mentioned this pull request Apr 25, 2020

Restyle Separate visualizations into their own package #4838

Closed

gabrieldutra added 4 commits April 25, 2020 20:10

Delete shared components

8af48f6

Remove antd from deps

efba9ae

Remove p-r-5 from table utils

8503dba

Remove visualization deps from package.json

bfbd8a2

arikfr reviewed Apr 26, 2020

View reviewed changes

Rename package and change its version

a9f2466

gabrieldutra force-pushed the visualizations-package branch 4 times, most recently from cff5ec4 to fa0cfbc Compare April 26, 2020 17:35

Test preinstall script

220ccad

gabrieldutra force-pushed the visualizations-package branch from fa0cfbc to 220ccad Compare April 26, 2020 17:37

Update Dockerfile build for frontend

e6d0d65

weekly-digest bot mentioned this pull request Apr 27, 2020

Weekly Digest (20 April, 2020 - 27 April, 2020) #4839

Closed

gabrieldutra added 2 commits April 28, 2020 10:27

Test adding dockerignore

5f56207

Update jest tests

ed61df1

gabrieldutra force-pushed the visualizations-package branch from 73307c4 to ed61df1 Compare April 28, 2020 13:28

gabrieldutra commented Apr 28, 2020

View reviewed changes

gabrieldutra added 2 commits April 28, 2020 11:24

Add step for jest tests

6711d29

Include viz-lib on dev commands

b0fcfd7

gabrieldutra force-pushed the visualizations-package branch from 7e5bd3d to b0fcfd7 Compare April 28, 2020 15:14

gabrieldutra commented Apr 28, 2020

View reviewed changes

gabrieldutra added 2 commits April 28, 2020 16:59

User prettier v1 for now

f2cdda0

Delete unused libs on the app

8f7b987

gabrieldutra marked this pull request as ready for review April 28, 2020 22:58

kravets-levko added the Frontend label Apr 29, 2020

kravets-levko added the Visualizations label Apr 29, 2020

Merge branch 'master' into visualizations-package

9915a91

gabrieldutra mentioned this pull request Apr 29, 2020

Basic project setup for visualizations extraction #4779

Closed

1 task

daniellangnet mentioned this pull request Apr 29, 2020

Added KPI visualization #4640

Open

1 task

gabrieldutra mentioned this pull request Apr 30, 2020

Fix visualizations with filters not showing selected values #4854

Merged

1 task

gabrieldutra added 2 commits May 3, 2020 13:46

Merge branch 'master' into visualizations-package

5ca9503

Add readme draft (to be finished)

66d7162

weekly-digest bot mentioned this pull request May 4, 2020

Weekly Digest (27 April, 2020 - 4 May, 2020) #4863

Closed

gabrieldutra added 3 commits May 4, 2020 19:17

Add getOptions to Editor

2505abc

Add required libraries and finish basic example

5e8124a

Bump version

8489794

arikfr merged commit fc246aa into master May 6, 2020

weekly-digest bot mentioned this pull request May 11, 2020

Weekly Digest (4 May, 2020 - 11 May, 2020) #4877

Closed

ktmud mentioned this pull request Aug 14, 2020

Add React hot refresh and refine viz lib build #5104

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate visualizations into their own package #4837

Separate visualizations into their own package #4837

gabrieldutra commented Apr 25, 2020 •

edited

Loading

arikfr left a comment

arikfr Apr 26, 2020

arikfr Apr 26, 2020

gabrieldutra Apr 28, 2020

kravets-levko Apr 29, 2020

gabrieldutra Apr 28, 2020

arikfr Apr 30, 2020

gabrieldutra Apr 30, 2020 •

edited

Loading

arikfr Apr 30, 2020

daniellangnet commented Apr 29, 2020

gabrieldutra commented Apr 29, 2020

gabrieldutra commented Apr 29, 2020

arikfr commented May 5, 2020

arikfr commented May 6, 2020

Separate visualizations into their own package #4837

Separate visualizations into their own package #4837

Conversation

gabrieldutra commented Apr 25, 2020 • edited Loading

What type of PR is this? (check all applicable)

Description

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

arikfr left a comment

Choose a reason for hiding this comment

arikfr Apr 26, 2020

Choose a reason for hiding this comment

arikfr Apr 26, 2020

Choose a reason for hiding this comment

gabrieldutra Apr 28, 2020

Choose a reason for hiding this comment

kravets-levko Apr 29, 2020

Choose a reason for hiding this comment

gabrieldutra Apr 28, 2020

Choose a reason for hiding this comment

arikfr Apr 30, 2020

Choose a reason for hiding this comment

gabrieldutra Apr 30, 2020 • edited Loading

Choose a reason for hiding this comment

arikfr Apr 30, 2020

Choose a reason for hiding this comment

daniellangnet commented Apr 29, 2020

gabrieldutra commented Apr 29, 2020

gabrieldutra commented Apr 29, 2020

arikfr commented May 5, 2020

arikfr commented May 6, 2020

gabrieldutra commented Apr 25, 2020 •

edited

Loading

gabrieldutra Apr 30, 2020 •

edited

Loading