Provide regl-based traces in the strict bundle #6083

yujin-wu · 2022-01-17T17:27:39Z

Issue

Regl traces (splom, scattergl, scatterpolargl, parcoords) are not CSP compliant.
cc: #897

Changes

Add 3 new traces - scattergl-strict, splom-strict, parcoords-strict, which are CSP compliant versions of the scattergl, splom and parcoords traces.
Added 4 traces to the strict build - scattergl-strict, scatterpolargl, parcoords-strict, splom-strict.
Added regl codegen tool, via npm run regl-codegen

TODO blocking

Update regl version after required regl PR is through Plotly - device-independent and stable codegen + codegen cache. regl-project/regl#637
Tests
Update Readme

…cluded them exclusively in the strict build.

…mplete

yujin-wu · 2022-01-19T18:58:23Z

So some background:

Every regl shader creates (hopefully the same) unique code. A PR is submitted in regl to make its codegen stable, meaning
- making sure that the same shader generates the same code on any device and on any codegen execution order (regl uses incrementing IDs to generate various things like variable names and ids)
Our regl traces end up using some fixed regl shaders.
We need tool to quickly generate code in case:
- our dependencies change the shaders in any slight way, or
- we update our regl traces in some way to use different shaders, or
- if regl updates.

Some iffy parts:

I have decieded to put the generated code into the src directory itself. This is so:
- building of bundles that need the codegen code can happen headlessly
- it would be more apparent when generated code is inconsistent across devices - every developer's computer, after running npm run regl-codegen, should produce no diffs.
  but idk if it's too polluting of the src dir?

package.json

This reverts commit 4fcf623.

package.json

…regl_codegen/style.css

archmoj · 2022-03-11T16:35:53Z

package.json

    "cibuild": "npm run empty-dist && npm run preprocess && node tasks/cibundle.js",
    "watch": "node tasks/watch.js",
    "lint": "eslint --version && eslint .",
    "lint-fix": "eslint . --fix || true",
    "log-new-func": "eslint --no-ignore --no-eslintrc --no-inline-config --rule '{no-new-func: error}' dist/plotly.js 2>&1 | ./tasks/show_eval_lines.sh",
-    "no-new-func": "eslint --no-ignore --no-eslintrc --no-inline-config --rule '{no-new-func: error}' $(find dist -type f -iname '*.js' | grep -v plotly-gl2d* | grep -v plotly-with-meta.* | grep -v plotly.js | grep -v plotly.min.js)",
+    "no-new-func": "eslint --no-ignore --no-eslintrc --no-inline-config --rule '{no-new-func: error}' $(find dist -type f -iname '*.js' | grep -v plotly-gl2d* | grep -v plotly-with-meta.* | grep -v plotly.js | grep -v plotly.min.js | grep -v plotly-strict.js | grep -v plotly-strict.min.js)",


Why those strict bundles are excluded from this test?
Please revert this change.
Thank you.

Hmm... so the Function.apply(...) ends up in the strict bundle but it never get called?
@alexcjohnson what's your suggestion here?

Yes indeed that's the reason. I admit it was hackier to exclude it here because ironically it is the whole point of the check.

To remove reference to it we would have to make a second build of regl right? Say, regl-strict? If the intention of the check is to catch mistakes then can this be an exception?

Yes, that makes sense. Let's leave this as is for now, but make an issue to come back to it with a second build of regl that can only be used with precompiled functions and does not include the codegen code at all, simply throwing an error if a matching precompiled function isn't included. For anyone that uses this in a CSP-restricted environment an error is what would happen anyway if for some reason we tried to generate a missing function, so that code is merely wasted bytes, not a vulnerability.

archmoj · 2022-03-11T16:45:05Z

src/traces/parcoords-strict/index.js

+var index = require('../parcoords/index');
+
+index.plot = require('../parcoords/plot-strict');
+


Let's duplicate parcoords/index content here and then replace plot with plot-strict in this file.
That way the content of parcoords/plot.js won't be included in the bundle.
On another note - wondering can't we move this index into the parcoords folder and name it index-strict instead?

wondering can't we move this index into the parcoords folder and name it index-strict instead?

A bunch of build stuff relies on traces having their own folders and I didn't want to mess around with that too much

That way the content of parcoords/plot.js won't be included in the bundle.

plot-strict is just a wrapper around plot after all and plot would need to be in both bundles.

.circleci/config.yml

Co-authored-by: Mojtaba Samimi <[email protected]>

tasks/stats.js

alexcjohnson

💃 Fantastic work, a team effort by @yujin-wu and @archmoj 🏆

Just a tiny comment on the bundle docs, then let's do it!

Co-authored-by: Alex Johnson <[email protected]>

archmoj · 2022-03-12T00:32:59Z

Thanks @yujin-wu for the remarkable contribution! 🎖️ 🏅 🥇

yujin-wu added 5 commits January 20, 2022 04:59

automated codegen

3b4bbc3

uncommitted generated traces

c5624ee

added strict version of parcoords, scattergl and splom traces, and in…

e958fe8

…cluded them exclusively in the strict build.

undo changes to dist

c9bcfb2

added draftlogs entry

ac4f861

yujin-wu force-pushed the regl-build-setup branch from 1799925 to ac4f861 Compare January 19, 2022 18:27

removed unused code in codegen tooling, and made it auto exit when co…

aec12ef

…mplete

yujin-wu requested review from archmoj and alexcjohnson January 19, 2022 18:50

archmoj reviewed Jan 24, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

yujin-wu added 2 commits January 26, 2022 22:10

handle updated regl cache mechanism

2c8f7c4

referenced regl from github PR

ebd90d5

yujin-wu requested a review from archmoj January 27, 2022 07:20

yujin-wu added 5 commits January 27, 2022 18:34

updated CONTRIBUTING with instructions for npm run regl-codegen

8fbcf15

added link to codegen location in CONTRIBUTING.md

6748211

clarified some workding in CONTRIBUTING.md

8ef4309

fixed package lock

2ab5976

.

b137e09

yujin-wu force-pushed the regl-build-setup branch from f9ea4cd to b137e09 Compare January 28, 2022 14:22

try with regl that has bumped gl to 4.9.2

ddf40e1

archmoj reviewed Feb 7, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

yujin-wu added 3 commits February 8, 2022 04:21

used hash of custom regl instead of branch name

4fcf623

Revert "used hash of custom regl instead of branch name"

4c03bb8

This reverts commit 4fcf623.

used hash of custom regl instead of branch name

bb6bdac

archmoj reviewed Feb 9, 2022

View reviewed changes

package.json Show resolved Hide resolved

yujin-wu added 4 commits February 11, 2022 00:04

try node 12 npm 6 and restored reg (oops)

45be643

try installing python2.7 first

b1b2866

.

7589382

[temporary] try without npm ls

9be5d47

yujin-wu added 2 commits March 12, 2022 03:10

restored erronously omitted node12 circleci test, removed duplicated …

1b114c4

…regl_codegen/style.css

Merge branch 'master' into regl-build-setup

666871e

archmoj reviewed Mar 11, 2022

View reviewed changes

.circleci/config.yml Outdated Show resolved Hide resolved

yujin-wu and others added 12 commits March 12, 2022 04:36

scatterpolargl-strict trace

abe7664

Update .circleci/config.yml

a49b892

Co-authored-by: Mojtaba Samimi <[email protected]>

bring back space in circleci config

b719179

move strict index files into regl traces

374b086

start with base_index without plot to add strict plot fns

ec82c09

lint src/traces/scatterpolargl/plot.js

862ff3e

revert tasks/partial_bundle.js

98d49be

cleanup tasks/util/constants.js

867c98b

strict no longer a partial bundle - generate at build step

f94aa45

fixup index-strict - cleanup confusing strict traces

3cecf47

fixup package-lock

2ec0545

fixup trace names in strict bundle

501e0a8

alexcjohnson reviewed Mar 11, 2022

View reviewed changes

tasks/stats.js Outdated Show resolved Hide resolved

alexcjohnson approved these changes Mar 11, 2022

View reviewed changes

archmoj and others added 4 commits March 11, 2022 17:20

update draftlogs for next minor

fd8a303

Update tasks/stats.js

3ba7dab

Co-authored-by: Alex Johnson <[email protected]>

update tasks/stats.js

98702c8

skip strict in the partial bundle script

e34df29

archmoj changed the title ~~Regl build setup~~ Provide regl-based traces in the strict bundle Mar 12, 2022

archmoj added the feature something new label Mar 12, 2022

archmoj merged commit bc95e66 into master Mar 12, 2022

archmoj deleted the regl-build-setup branch March 12, 2022 00:12

archmoj mentioned this pull request Mar 15, 2022

Regenerate functions for regl-based traces in the strict bundle #6141

Merged

CallumNZ mentioned this pull request Feb 20, 2023

Can't make custom bundle with strict scattergl trace #6496

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide regl-based traces in the strict bundle #6083

Provide regl-based traces in the strict bundle #6083

yujin-wu commented Jan 17, 2022 •

edited

Loading

yujin-wu commented Jan 19, 2022 •

edited

Loading

archmoj Mar 11, 2022

archmoj Mar 11, 2022

yujin-wu Mar 11, 2022

alexcjohnson Mar 11, 2022

archmoj Mar 11, 2022

yujin-wu Mar 11, 2022 •

edited

Loading

alexcjohnson left a comment

archmoj commented Mar 12, 2022

		var index = require('../parcoords/index');

		index.plot = require('../parcoords/plot-strict');

Provide regl-based traces in the strict bundle #6083

Provide regl-based traces in the strict bundle #6083

Conversation

yujin-wu commented Jan 17, 2022 • edited Loading

Issue

Changes

TODO blocking

yujin-wu commented Jan 19, 2022 • edited Loading

archmoj Mar 11, 2022

Choose a reason for hiding this comment

archmoj Mar 11, 2022

Choose a reason for hiding this comment

yujin-wu Mar 11, 2022

Choose a reason for hiding this comment

alexcjohnson Mar 11, 2022

Choose a reason for hiding this comment

archmoj Mar 11, 2022

Choose a reason for hiding this comment

yujin-wu Mar 11, 2022 • edited Loading

Choose a reason for hiding this comment

alexcjohnson left a comment

Choose a reason for hiding this comment

archmoj commented Mar 12, 2022

yujin-wu commented Jan 17, 2022 •

edited

Loading

yujin-wu commented Jan 19, 2022 •

edited

Loading

yujin-wu Mar 11, 2022 •

edited

Loading