Feature/st lib #6692
Feature/st lib #6692
Conversation
…derer (#6623) This PR is the first milestone to determine the minimal number of exports needed for app to consume + VerticalBlock + ElementNodeRenderer. Most of the changes are import path changes. Some of the more notable changes to look through are How the @streamlit/lib is built. It's built through the monorepo config for babel (https://babeljs.io/docs/config-files/#monorepos). a. The babel process works by using a custom preset that is mainly babel-preset-react-app but slightly modified in order to make sure glide data grid code is es6 compatible and hard coding isTypeScriptEnabled to True. b. The other keys are adding in the emotion plugin for styled-components, ignoring proto, vendor, test files, and telling babel that frontend is the root directory for babel config. The actual index.ts that is added to frontend/lib Some things to note that will be punt down but worked on later: We also need to look into why protobuf can't be compiled through tsc. Cleaning frontend/app/package.json Cleaning frontend/app/yarn.lock Cleaning frontend/lib/scripts/create.js Determine what's going wrong with globalStyles typing Determine what's going wrong with styled-components typing for EmotionTheme
…ied through cp command. (#6635) Tim noticed that I was copying the protobuf.js and protobuf.d.ts instead of compiling it through babel. That causes problems because then you have to import by doing import { ... } from "@streamlit/lib/dist/proto" which doesn't make any sense. The reason why I did that was because I was getting an error on first try: `src/proto.js:4:1 - error TS9005: Declaration emit for this file requires using private name 'Radii'. An explicit type annotation may unblock declaration emit. 4 import * as $protobuf from "protobufjs/minimal"; ~~~~~~ src/proto.js:4:1 - error TS9005: Declaration emit for this file requires using private name 'Radio'. An explicit type annotation may unblock declaration emit. 4 import * as $protobuf from "protobufjs/minimal";` I just removed the proto files and did a make protobuf and then tried exporting and that worked, as it should.
There are going to be 2 emotion.d.ts. Our @streamlit/lib has our own theming and then our app will simply just be consuming from @streamlit/lib which makes sense.
The current folder structure is frontend/app/src/app. We should make it so that it's frontend/app/src and remove the extra app directory. This PR also fixes the audit_license script.
This PR is to fix eslint for app and lib. This also adds dependencies that are necessary to lib's package.json. This is done through eslint noticing the necessary dependencies. This PR also does some linting and also does a merge from develop as we need the BaseButton refactoring to stop duplication of exports from the root index.ts. In addition, this PR fixes the type checking github actions by adding new commands.
… into feature/st-lib
This is to fix yarn test. This simply adds new scripts through the yarn workspace command to run app and lib tests. Currently, the lib testing works by mainly using the same scripts that CRA uses for transpiling typescript to javascript. There is one other thing to note: It looks like if you were to upgrade to 17.0.1 supposedly for react-dom, it would fix jest tests for app to not hang (facebook/react#20756 (comment)). However, jest was still hanging so I just did a delete MessageChannel within src/setupTests.js and we can remove that when we upgrade to React 18.
* Remove unused app dep / dev dep and declarations.d.ts * Remove d3 color resolution (#6714) * Add dependencies removed from app to lib/package.json * Add pbjs cli * Readd notices
Since @streamlit/lib is separated out to be its own separate lib, we need a script to constantly rebuild when the files change. Will also need to update the contributing wiki after this is merged to develop.
There was a problem hiding this comment.
Seems like this does belong but not sure why this is saying it's added
There was a problem hiding this comment.
We can see that this snapshot is here deleted
There was a problem hiding this comment.
Overall LGTM. But I wasn't following this project too closely, so I don't have too much context on this.
Some questions:
-
What exactly will be our new development workflow with this change? E.g. I usually have the following setup: One terminal with
streamlit run ...and one terminal withyarn startinfrontend. And it will hot reload whenever I do any frontend change. -
How do I build a distribution with this change? I currently do: 1.
make protobuf2.make frontend3.make distributionin the Streamlit root folder. -
This isn't necessarily something specific to this change. But I think the three snapshot files that I have commented on above are kind of broken and we should eventually remove those. I believe there isn't any real value in doing a snapshot on the enzyme mount as done in those cases. But this could also be done in a separate PR.
There was a problem hiding this comment.
Can we remove this? See comment above.
frontend/lib/src/components/elements/Spinner/__snapshots__/Spinner.test.tsx.snap
Show resolved
Hide resolved
...nd/lib/src/components/elements/ExceptionElement/__snapshots__/ExceptionElement.test.tsx.snap
Show resolved
Hide resolved
one terminal for
I just tried
I will make a separate PR! |
I have added a discussion topic on that for our next standup. So no rush to get a PR on that out :)
Does this mean that I need to refresh the browser manually to see the change? |
This reverts commit 87b7f13.
Yes :/. I was planning to add a fast-follow PR to look through tabs with localhost:3000 with pyautogui and refresh it. |
|
I'm not too worried about manually refreshing the tab on changes. But I still see some risk that it negatively impacts the overall frontend development flow. E.g. how fast does it compile after a change? Is it about the same as in the current hot reloading? Do we see the full stack traces on frontend exceptions, or only the minified versions? ... We definitely should take that into the next standup discussion before merging. And maybe you giving a demo of how local frontend development will work with these changes. |
We definitely should take that into the next standup discussion before merging. And maybe you giving a demo of how local frontend development will work with these changes. Sure I can give a demo.
I would say the compilation time is a good amount slower since you have to build the library which in itself is just slow and expected to be some what slow. I would say the compilation will likely take a 30 seconds - a minute.
Let me test that and get back to you or write up something during standup. |
) <!--⚠️ BEFORE CONTRIBUTING PLEASE READ OUR CONTRIBUTING GUIDELINES! https://github.com/streamlit/streamlit/wiki/Contributing --> ## Describe your changes - Change imports for `app` to `@streamlit/app/src...` as we removed the `ModuleScopePlugin` which allows us to import out of `app/src`. - Change imports for `lib` to absolute imports - add `tsconfig.dev.json` that is specifically for developement - change `tsconfig.json` for `lib` and `app` - change `.eslintrc` to `.eslintrc.js` in order to gain access to `__dirname` to make typescript configs relative to each project ## GitHub Issue Link (if applicable) ## Testing Plan Manually testing - Explanation of why no additional tests are needed - Unit Tests (JS and/or Python) - E2E Tests - Any manual testing needed? yes https://github.com/streamlit/streamlit/assets/16749069/cc0a5f96-f8cd-487b-b70e-6a4e5823f0e7 --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.
| }) | ||
| // Remove ModuleScopePlugin which throws when we try to import something | ||
| // outside of src/. | ||
| webpackConfig.resolve.plugins.pop() |
There was a problem hiding this comment.
I would prefer a more specific search to remove by name. See further up in the code.
This will make this more resilient to react-scripts.
There was a problem hiding this comment.
Will make a separate PR to develop after merging.
* develop: Feature: st.toast (streamlit#6783) Bump semver from 5.7.1 to 5.7.2 in /frontend (streamlit#6982) Add host communication e2e tests (streamlit#6806) Re-add homepage to package.json (streamlit#6987) Remove unnecessary code and Add rm commands to make clean (streamlit#6980) Allow setting placeholder for st.selectbox (streamlit#6913) Allow setting placeholder for st.multiselect (streamlit#6901) Also use bottom padding in embedded mode for chat input (streamlit#6979) Feature/st lib (streamlit#6692) Remove unused import (streamlit#6977) Release 1.24.1 (streamlit#6965) Update st.audio/st.video docstrings (streamlit#6964) Slightly simplify bug report template (streamlit#6972) Fix baseweb warnings by using longhand properties (streamlit#6976)
* Minimal exports for app to consume + VerticalBlock and ElementNodeRenderer (streamlit#6623) This PR is the first milestone to determine the minimal number of exports needed for app to consume + VerticalBlock + ElementNodeRenderer. Most of the changes are import path changes. Some of the more notable changes to look through are How the @streamlit/lib is built. It's built through the monorepo config for babel (https://babeljs.io/docs/config-files/#monorepos). a. The babel process works by using a custom preset that is mainly babel-preset-react-app but slightly modified in order to make sure glide data grid code is es6 compatible and hard coding isTypeScriptEnabled to True. b. The other keys are adding in the emotion plugin for styled-components, ignoring proto, vendor, test files, and telling babel that frontend is the root directory for babel config. The actual index.ts that is added to frontend/lib Some things to note that will be punt down but worked on later: We also need to look into why protobuf can't be compiled through tsc. Cleaning frontend/app/package.json Cleaning frontend/app/yarn.lock Cleaning frontend/lib/scripts/create.js Determine what's going wrong with globalStyles typing Determine what's going wrong with styled-components typing for EmotionTheme * Change exports for protobuf to be included in index.ts instead of copied through cp command. (streamlit#6635) Tim noticed that I was copying the protobuf.js and protobuf.d.ts instead of compiling it through babel. That causes problems because then you have to import by doing import { ... } from "@streamlit/lib/dist/proto" which doesn't make any sense. The reason why I did that was because I was getting an error on first try: `src/proto.js:4:1 - error TS9005: Declaration emit for this file requires using private name 'Radii'. An explicit type annotation may unblock declaration emit. 4 import * as $protobuf from "protobufjs/minimal"; ~~~~~~ src/proto.js:4:1 - error TS9005: Declaration emit for this file requires using private name 'Radio'. An explicit type annotation may unblock declaration emit. 4 import * as $protobuf from "protobufjs/minimal";` I just removed the proto files and did a make protobuf and then tried exporting and that worked, as it should. * Simple fix for globalStyles (streamlit#6651) * Fix typing issues for styled-components (streamlit#6652) There are going to be 2 emotion.d.ts. Our @streamlit/lib has our own theming and then our app will simply just be consuming from @streamlit/lib which makes sense. * remove extra app directory in frontend dir (streamlit#6653) The current folder structure is frontend/app/src/app. We should make it so that it's frontend/app/src and remove the extra app directory. This PR also fixes the audit_license script. * Remove unnecessary files * Fix naming of directories and files * Fix BaseColorPicker directory * Add Fixes that should exist in feature/st-lib * Fix Eslint and Type-checking for st-lib (streamlit#6675) This PR is to fix eslint for app and lib. This also adds dependencies that are necessary to lib's package.json. This is done through eslint noticing the necessary dependencies. This PR also does some linting and also does a merge from develop as we need the BaseButton refactoring to stop duplication of exports from the root index.ts. In addition, this PR fixes the type checking github actions by adding new commands. * Fix notices (streamlit#6676) * Clean up audit_frontend_licenses * Fix/yarn test (streamlit#6681) This is to fix yarn test. This simply adds new scripts through the yarn workspace command to run app and lib tests. Currently, the lib testing works by mainly using the same scripts that CRA uses for transpiling typescript to javascript. There is one other thing to note: It looks like if you were to upgrade to 17.0.1 supposedly for react-dom, it would fix jest tests for app to not hang (facebook/react#20756 (comment)). However, jest was still hanging so I just did a delete MessageChannel within src/setupTests.js and we can remove that when we upgrade to React 18. * Fix regex that I accidentally forgot to commit (streamlit#6698) * Fix/cypress (streamlit#6699) * Remove unused app dep / dev dep and declarations.d.ts (streamlit#6713) * Remove unused app dep / dev dep and declarations.d.ts * Remove d3 color resolution (streamlit#6714) * Add dependencies removed from app to lib/package.json * Add pbjs cli * Readd notices * Add watch lib script (streamlit#6730) Since @streamlit/lib is separated out to be its own separate lib, we need a script to constantly rebuild when the files change. Will also need to update the contributing wiki after this is merged to develop. * Get rid of yarn start warnings and clean Makefile (streamlit#6739) * Remove streamlit.css. I'm not sure where it came from * Fix yarn buildLib * Fix eslintrc * Fix codeql * Add notices and yarn.lock * Fix glide-data-grid versions * Fix nits * Fix nits * Lint and cleanup * Minor cleanup for Block utils as comments got removed * Remove accidentally committed file * Revert "Remove accidentally committed file" This reverts commit 87b7f13. * Fix yarn lock * Fix compilation errors * Fix lint * Fix notices * Remove unnecessary package.json dependencies * Fix package.json versions * Fix yarn.lock and streamlit dialog snapshots * Fix notices * Have faster hot reloading and absolute imports for @streamlit/lib (streamlit#6961) <!--⚠️ BEFORE CONTRIBUTING PLEASE READ OUR CONTRIBUTING GUIDELINES! https://github.com/streamlit/streamlit/wiki/Contributing --> ## Describe your changes - Change imports for `app` to `@streamlit/app/src...` as we removed the `ModuleScopePlugin` which allows us to import out of `app/src`. - Change imports for `lib` to absolute imports - add `tsconfig.dev.json` that is specifically for developement - change `tsconfig.json` for `lib` and `app` - change `.eslintrc` to `.eslintrc.js` in order to gain access to `__dirname` to make typescript configs relative to each project ## GitHub Issue Link (if applicable) ## Testing Plan Manually testing - Explanation of why no additional tests are needed - Unit Tests (JS and/or Python) - E2E Tests - Any manual testing needed? yes https://github.com/streamlit/streamlit/assets/16749069/cc0a5f96-f8cd-487b-b70e-6a4e5823f0e7 --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license. * Fix minor comments from Lukas
This PR is to separate the designated streamlit components to a library. The apis that can should be consumed from this library will be
VerticalBlock,ElementNodeRendererand themes from Streamlit if need be. For full api, visitfrontend/lib/index.ts.In order to do this, import paths, github actions, local development flows will change and packaging of the library must happen.
src/apptoapp/src. The same will happen tolib.make protobufandmake frontend-liband some directories will be changed.appstill uses CRA but lib has its own bundling and compilation.babelandtsc.Babelis so our tyepscript code can be used in the browser by transforming our typescript code into browser javascript.Tscis to emit type declarations so that another person can use our library as typescript. We also do acpcommand that copies our auto-generated proto files to our library.yarn buildLibfirst.yarn start; however, you will also have to runpython scripts/watch_frontend_lib.pyormake watch-frontend-libfrom the streamlit root directory to hot reload the lib code. This occurs because @streamlit/lib is its own library. We shall iterate on this workflow later on.libandappwill also have separate jest configs (though they are pretty similar) and tsconfig.json (typescript checks).