Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: using MessageChannel prevents node.js process from exiting #20756

Closed
just-boris opened this issue Feb 7, 2021 · 15 comments · Fixed by #20834 or #20906
Closed

Bug: using MessageChannel prevents node.js process from exiting #20756

just-boris opened this issue Feb 7, 2021 · 15 comments · Fixed by #20834 or #20906

Comments

@just-boris
Copy link
Contributor

just-boris commented Feb 7, 2021


Edited: Here's how to fix this: #20756 (comment).


React version: 17.0.1 (latest)

Steps To Reproduce

This code finishes properly in node.js 14, but holds the process open in node.js 15

global.window = global; // simulate JSDOM
require('scheduler');

Since node.js 15, there is a global MessageChannel object now, which prevents node event loop from exiting. To let the process shut down properly, it should either call port.close() or port.unref()

This becomes an issue when testing React code within JSDOM environment, as the test process cannot exit properly.

The current behavior

The process with JSDOM tests never finishes

The expected behavior

The process finishes properly

@just-boris just-boris added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Feb 7, 2021
@rickhanlonii
Copy link
Member

Nice find! Do you want to submit a PR to fix?

@rickhanlonii rickhanlonii added Component: Scheduler good first issue and removed Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug labels Feb 10, 2021
@patil2097
Copy link

I would love to help you on fixing this problem.Can you guide me in submitting a fix? I am really new to open source. @rickhanlonii

@amensum
Copy link

amensum commented Feb 10, 2021

@rickhanlonii
Can I take this issue for try to fix it and send PR?

@amensum
Copy link

amensum commented Feb 10, 2021

Issue example:

var { JSDOM } = require('jsdom')

var { window } = new JSDOM()

global.window = window
global.document = window.document

require('scheduler')

@rickhanlonii
Copy link
Member

Hey all, anyone can take this issue, I'll review the first PR.

The problem is that this feature detection is too naive. In node environments, we should use unstable_no_dom and in browser environments we should use the browser scheduler.

@gaearon
Copy link
Collaborator

gaearon commented Feb 17, 2021

We're going with #20834 as a likely fix in 17.1.0, but it will need more testing.

@gaearon
Copy link
Collaborator

gaearon commented Feb 17, 2021

How to fix this

If you can upgrade to React 17.1.0

When React 17.1.0 comes out, it will contain a fix for this. So upgrading would solve this problem.

(At the time this comment is written, it's not out yet, but I expect it to be out soon.)

If you can't upgrade to React 17.1.0

npm i react-16-node-hanging-test-fix 

and add this to your test setup file as THE VERY FIRST IMPORT:

import 'react-16-node-hanging-test-fix';

// All other imports go after it:
import React from 'react'
// ...

(For example, Create React App projects use src/setupTests.js as the test setup file, otherwise you'll need to refer to your test runner's documentation. In Jest, the option is called setupFiles).

This should fix the hanging on Node 15+ with jsdom on older versions of React. We don't plan to be updating them because there's too much risk of breaking something else, as this detection is subtle and there are many possible edge cases.

The package with the fix will print a warning asking you to remove it after you upgrade to React 17.1.0. This is because the fix itself is to disable Node's MessageChannel implementation, which is not ideal in long term.


Let me know if you run into more problems! I'll keep the issue for now until we release 17.1.0.

@just-boris
Copy link
Contributor Author

Brilliant fix :)

delete global.MessageChannel;

Would it make sense to let Node.js devs know that this is happening? Perhaps, they could consider undoing the change in Node 16.

@gaearon
Copy link
Collaborator

gaearon commented Feb 18, 2021

I've briefly chatted with Node folks but it seems pretty difficult to change (and would be a breaking change). In our case, setImmediate is closer to the semantics we need anyway, so it makes sense as a fix. Just a bit risky to port to all the past versions in case people run bad polyfills etc.

rithvikvibhu added a commit to rithvikvibhu/bob-wallet that referenced this issue Feb 4, 2022
rithvikvibhu added a commit to rithvikvibhu/bob-wallet that referenced this issue Feb 4, 2022
pinheadmz pushed a commit to pinheadmz/bob-wallet that referenced this issue Feb 9, 2022
chikeichan pushed a commit to kyokan/bob-wallet that referenced this issue Feb 15, 2022
* pkg: update dependencies webpack/babel/electron

* webpack: update configs to migrate to webpack 5

* pkg: update babel config for updated electron

* app: stub globalThis for global

* sentry: bypass check for isPackaged in renderer: electron / remote

* app: replace electron.remote with updated @electron/remote

* deps: update sentry

* build: replace loaders in webpack configs

* ci: update to node 16

* tests: webpack config and react bug

More info:
facebook/react#20756 (comment)

* ci: add windows build and package

* util: replace nameChecker with hsd rules module

* pkg: document mac arm64 and update package.json scripts

* docs: update BUILD.md with new mac package cmd

* pkg: move `react-16-node-hanging-test-fix` to devDep

* pkg: update non-breaking deps

* sentry: load sentry and app based on process

Co-authored-by: Rithvik Vibhu <rithvikvibhu@gmail.com>
lukeburns pushed a commit to lukeburns/bob-wallet that referenced this issue Feb 15, 2022
localnerve added a commit to localnerve/element-size-reporter that referenced this issue Mar 23, 2022
@jarredt
Copy link

jarredt commented Aug 25, 2022

@gaearon -- I think the react-16-node-hanging-test-fix file may have a critical typo:

if (semverGt(version, '17.0.1')) {
  console.error(
    'The `react-16-node-hanging-test-fix` package is no longer needed ' +
    'with React ' + version + ' and may cause issues. Remove this import.'
  )
}

Shouldn't the if be evaluating for "greater than or equal to 17.1.0"? Per your comment and the comment at the top of the package file, the issue is fixed starting with 17.1.0.

Right now it's evaluating "greater than 17.0.1" -- so I'm getting the warning for 17.0.2 even though the issue isn't fixed there.

@jarredt
Copy link

jarredt commented Aug 25, 2022

Also... it looks like maybe the fix actually didn't land until React 18, so maybe needs to be changed to 18.0.0?

Ref: Mention of the fixing PR (#20834) in #24195.

@rickhanlonii
Copy link
Member

rickhanlonii commented Aug 25, 2022

@jarredt what version of react-dom and scheduler are you using? I think the issue is that the code checks react for 17.0.1, but the fix is actually in the scheduler package, through react-dom. If you update react-dom to 17.0.1, the issue should be fixed and you don't need react-16-node-hanging-test-fix.

@jarredt
Copy link

jarredt commented Aug 29, 2022

@rickhanlonii -- ah, so we're using 20.0.2 for react-dom, but one of our storybook-related dependencies is bringing in 0.19.1 as a transitive of react-dom@16.14.0. I assume that may be the issue I'll see if I can get that bumped or excluded from executing in our tests.

@andrecrimb
Copy link

@jarredt did you manage to get around this issue? We are facing the same problem.

@jarredt
Copy link

jarredt commented Sep 9, 2022 via email

tjzel added a commit to software-mansion/react-native-reanimated that referenced this issue Apr 24, 2023
tjzel added a commit to software-mansion/react-native-reanimated that referenced this issue May 9, 2023
## Summary

- Renamed all tests with their respective TypeScript counterparts.
- Fixed jestUtils to properly allow strict checking of AnimatedStyle.
- Added type declaration emitting to plugin.
- Moved building plugin files before running TypeScript on the project
(otherwise TypeScript tests would fail during type-checking).
- Added test for jestUtils.
- Bumped `jest` version and added `@types/react-test-renderer`.

---
Added to `jest-setup.js`:
```diff
+    delete global.MessageChannel;
```
This line is coming from `react-16-node-hanging-test-fix`. It fixes an
error of open handles in `@testing-library/react-native` function
`render` and jest going 😵.
[Reference](facebook/react#20756).

## Test plan

`yarn && yarn jest --detectOpenHandles` ftw
willhuang1997 added a commit to streamlit/streamlit that referenced this issue May 16, 2023
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.
fluiddot pushed a commit to wordpress-mobile/react-native-reanimated that referenced this issue Jun 5, 2023
## Summary

- Renamed all tests with their respective TypeScript counterparts.
- Fixed jestUtils to properly allow strict checking of AnimatedStyle.
- Added type declaration emitting to plugin.
- Moved building plugin files before running TypeScript on the project
(otherwise TypeScript tests would fail during type-checking).
- Added test for jestUtils.
- Bumped `jest` version and added `@types/react-test-renderer`.

---
Added to `jest-setup.js`:
```diff
+    delete global.MessageChannel;
```
This line is coming from `react-16-node-hanging-test-fix`. It fixes an
error of open handles in `@testing-library/react-native` function
`render` and jest going 😵.
[Reference](facebook/react#20756).

## Test plan

`yarn && yarn jest --detectOpenHandles` ftw
willhuang1997 added a commit to streamlit/streamlit that referenced this issue Jul 10, 2023
* Minimal exports for app to consume + VerticalBlock and ElementNodeRenderer (#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. (#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 (#6651)

* Fix typing issues for styled-components (#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 (#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 (#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 (#6676)

* Clean up audit_frontend_licenses

* Fix/yarn test (#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 (#6698)

* Fix/cypress (#6699)

* Remove unused app dep / dev dep and declarations.d.ts (#6713)

* 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

* Add watch lib script (#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 (#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 (#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
zatteo added a commit to cozy/cozy-flagship-app that referenced this issue Nov 22, 2023
We have a lot of open handles flooding the end of our test results. It seems that they are due to a React issue fixed in 18+.

Dan Abramov wrote a small package to fix the issue temporarly. I added it here manually because it has an error in the semver version check. This code should be removed when upgrading React to 18+.

facebook/react#20756 (comment)
zatteo added a commit to cozy/cozy-flagship-app that referenced this issue Dec 5, 2023
We have a lot of open handles flooding the end of our test results. It seems that they are due to a React issue fixed in 18+.

Dan Abramov wrote a small package to fix the issue temporarly. I added it here manually because it has an error in the semver version check. This code should be removed when upgrading React to 18+.

facebook/react#20756 (comment)
zatteo added a commit to cozy/cozy-flagship-app that referenced this issue Dec 5, 2023
We have a lot of open handles flooding the end of our test results. It seems that they are due to a React issue fixed in 18+.

Dan Abramov wrote a small package to fix the issue temporarly. I added it here manually because it has an error in the semver version check. This code should be removed when upgrading React to 18+.

facebook/react#20756 (comment)
zatteo added a commit to cozy/cozy-flagship-app that referenced this issue Dec 5, 2023
We have a lot of open handles flooding the end of our test results. It seems that they are due to a React issue fixed in 18+.

Dan Abramov wrote a small package to fix the issue temporarly. I added it here manually because it has an error in the semver version check. This code should be removed when upgrading React to 18+.

facebook/react#20756 (comment)
eric-skydio pushed a commit to eric-skydio/streamlit that referenced this issue Dec 20, 2023
* 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
zyxue pushed a commit to zyxue/streamlit that referenced this issue Mar 22, 2024
* 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
zyxue pushed a commit to zyxue/streamlit that referenced this issue Apr 16, 2024
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants