Add Bokeh as a side effect import #6512

willhuang1997 · 2023-04-19T03:59:37Z

This PR is to remove Bokeh in the index.html and add them as sideEffect imports which means mainly mutating global objects. This allows for better lazy loading for bokeh. By removing Bokeh from index.html and the downloading of bokeh scripts, some other github actions and such things had to change.

willhuang1997 · 2023-04-19T04:00:05Z

.github/codeql-config.yml

 paths:
  - lib/streamlit
  - frontend/src
+paths-ignore:


ignore min.js files for codeql

willhuang1997 · 2023-04-19T04:00:25Z

frontend/package.json

+    "start": "env NODE_OPTIONS=--max_old_space_size=8192 ESLINT_NO_DEV_ERRORS=true craco start",
+    "build": "env NODE_OPTIONS=--max_old_space_size=8192 GENERATE_SOURCEMAP=false craco build",
+    "buildFast": "env NODE_OPTIONS=--max_old_space_size=8192 GENERATE_SOURCEMAP=false BUILD_AS_FAST_AS_POSSIBLE=true craco build",
+    "test": "env NODE_OPTIONS=\"--experimental-worker --max_old_space_size=8192\" craco test --env=./jsdom-polyfill-env.js --runTestsByPath ${TESTPATH}",


See comments in setupTests.js for why this is needed

willhuang1997 · 2023-04-19T04:37:47Z

Makefile

 	./scripts/append_license.sh frontend/src/assets/img/Material-Icons.LICENSE
 	./scripts/append_license.sh frontend/src/assets/img/Open-Iconic.LICENSE
-	./scripts/append_license.sh frontend/public/vendor/bokeh/bokeh-LICENSE.txt
+	./scripts/append_license.sh frontend/src/components/elements/BokehChart/vendor/bokeh-LICENSE.txt


I'm not sure if this should move from public/vendor but if it should, I can move it back. I was thinking we don't have it in the index.html so we don't need it there anymore.

it's not a huge deal (and others may disagree), but I do think we should keep all vendored files in that top-level vendor/ directory. It's the pattern we're using already, and it also means that there's a simple one-stop shop for anyone trying to figure out what our vendored deps are.

(So: let's move the contents of src/components/elements/BokehChart/vendor into public/vendor)

willhuang1997 · 2023-04-19T04:38:03Z

.gitignore


-# Preloaded, minified js (downloaded via script due to dependencies having
-# issues with babel transpilation).
-frontend/public/vendor/bokeh/


We don't download the scripts anymore so we don't need.

tconkling

This is great! Because there's some non-standard trickiness involved here, the PR could really benefit from clearer, and more complete, explanations of what's happening.

(We didn't have this explanation in the original code, which led to you doing a lot of research that the original Bokeh-in-Streamlit implementor had surely already done but neglected to document. I'd love to avoid that scenario again.)

Also, let's move all the vendored files back up to the top-level vendor directory.

tconkling · 2023-04-19T17:35:39Z

Makefile

 	./scripts/append_license.sh frontend/src/assets/img/Material-Icons.LICENSE
 	./scripts/append_license.sh frontend/src/assets/img/Open-Iconic.LICENSE
-	./scripts/append_license.sh frontend/public/vendor/bokeh/bokeh-LICENSE.txt
+	./scripts/append_license.sh frontend/src/components/elements/BokehChart/vendor/bokeh-LICENSE.txt


it's not a huge deal (and others may disagree), but I do think we should keep all vendored files in that top-level vendor/ directory. It's the pattern we're using already, and it also means that there's a simple one-stop shop for anyone trying to figure out what our vendored deps are.

(So: let's move the contents of src/components/elements/BokehChart/vendor into public/vendor)

frontend/.eslintrc

tconkling · 2023-04-19T17:40:27Z

frontend/src/components/elements/BokehChart/BokehChart.test.tsx

-  embed: {
-    embed_item: jest.fn(),
+import Bokeh from "./vendor/bokeh.esm.js"
+jest.mock("./vendor/bokeh.esm.js", () => ({


can you add a comment above the mock statement explaining why these particular mocks are needed?

tconkling · 2023-04-19T17:46:18Z

frontend/src/components/elements/BokehChart/BokehChart.tsx

+// bokeh doesn't play well with babel / cra (https://github.com/bokeh/bokeh/issues/10658)
+// have consumers provide their own bokeh minified implementation
+// bokeh.esm is renamed because has "import main from “./bokeh.esm.js"
+import Bokeh from "./vendor/bokeh.esm"


Yes, thank you for adding this comment! Can you reword it for clarity? Let's be as explicit as possible, so that future readers can understand this decision.

The first line: "We import Bokeh from a vendored source file, because it doesn't play well with Babel (...)"

The second line: I don't understand what this means. Is it a complete sentence or part of the previous sentence? Can it be reworded so that someone who has no context can understand?

Same with the third line - I don't understand what it means. "bokeh.esm is renamed" - renamed from what? (Presumably this means that we've renamed from the file, but let's make that explicit.) Why does "import main from ..." require renaming? What's the problem being solved?

tconkling · 2023-04-19T17:56:10Z

frontend/src/setupTests.js

+// needed for BokehChart.test.tsx as we import bokeh scripts that use message channel
+// https://github.com/jsdom/jsdom/issues/2448
+// need to add --experimental-worker in node options craco test cli command


This comment would also benefit from a clarity pass. Generally, sentence fragments and no punctuation isn't an issue in comments. But in this case, these are, I assume, two separate ideas ("We need to add the window.MessageChannel import here because..." and "We need to also add this --experimental-worker param elsewhere because").

Can you:

Add some punctuation (or an additional line break or whatever) so it's clear that these are two separate things being communicated

Rework that second point "need to add --experimental-worker". Who needs to add it? (Presumably you've already added it, so it's more "Additionally, we also modified such-and-such file to add such and such params" - it's not an imperative statement that the reader needs to act on.)

"node options craco test cli command" sounds like a series of disconnected words. My assumption is, you edited a specific file with that specific param in order to get bokeh tests to work. Can this comment instead say something like "Additionally: the test command in package.json uses the --experimental-worker node option, which is required to make Bokeh tests work."

frontend/src/vendor/bokeh/bokeh-api-2.4.3.esm.min.js

tconkling

Updated comments look great, thanks!

tconkling · 2023-04-19T19:43:13Z

(Oh, looks like the CodeQL ignore rules need to be updated for the Bokeh move into src/vendor)

vdonato

+1 for the change to the .github repo

* develop: Add Bokeh as a side effect import to modify globals instead of having it in index.html(streamlit#6512)

willhuang1997 added 15 commits April 17, 2023 15:41

Current

1e0ac6c

Add new bokeh files but it's taking forever to yarn start

cec39a6

Current

e0c993f

Working with help of Jichan

734b1f5

cleanup

80ddeee

Readd tsconfig to fix linter

02f68d8

Remove tsconfig because imports are followed

559e326

Fix Bokeh Tests

4ef9d60

Stop checking for js files

c098c3b

Revert it because it doesn't work

f5ab9e3

Fix lint error

e1552fc

Ignore vendor files for codeql

9104e0b

cleanup bokeh test

ae50b9e

Fix naming

762c33c

Fix test by readding mockClear

4c31d64

willhuang1997 commented Apr 19, 2023

View reviewed changes

willhuang1997 added 3 commits April 18, 2023 21:06

Fix test lint

66ea6b0

Fix codeql yaml lint

97ee516

Fix license and fix makefile

d730751

willhuang1997 commented Apr 19, 2023

View reviewed changes

willhuang1997 marked this pull request as ready for review April 19, 2023 10:07

willhuang1997 requested review from kmcgrady, mayagbarnes and vdonato as code owners April 19, 2023 10:07

willhuang1997 added the security-assessment-completed Security assessment has been completed for PR label Apr 19, 2023

tconkling approved these changes Apr 19, 2023

View reviewed changes

willhuang1997 mentioned this pull request Apr 19, 2023

Move public vendored files to src vendor #6518

Closed

Move bokeh min files to src/vendor and change imports

acf324c

willhuang1997 added 2 commits April 19, 2023 11:55

Reclarify comments

060ea27

fix directory path for bokeh license in makefile

4ae7a6d

github-advanced-security bot found potential problems Apr 19, 2023

View reviewed changes

tconkling approved these changes Apr 19, 2023

View reviewed changes

vdonato approved these changes Apr 19, 2023

View reviewed changes

willhuang1997 added 2 commits April 19, 2023 13:08

Fix codeql

44eba16

Change comment to be more clear and concise

2a014fe

willhuang1997 merged commit b8175e4 into develop Apr 19, 2023

tconkling added a commit to tconkling/streamlit that referenced this pull request Apr 19, 2023

Merge branch 'develop' into tim/FrontendAppLibSplit

f692005

* develop: Add Bokeh as a side effect import to modify globals instead of having it in index.html(streamlit#6512)

vdonato deleted the refactorBokeh branch November 2, 2023 00:00

Add Bokeh as a side effect import #6512

Add Bokeh as a side effect import #6512

Uh oh!

Conversation

willhuang1997 commented Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willhuang1997 Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tconkling left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tconkling left a comment

Choose a reason for hiding this comment

Uh oh!

tconkling commented Apr 19, 2023

Uh oh!

vdonato left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

willhuang1997 commented Apr 19, 2023 •

edited

Loading

willhuang1997 Apr 19, 2023 •

edited

Loading