You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "jgoz (via GitHub)" <gi...@apache.org> on 2023/06/06 22:58:08 UTC
[GitHub] [druid] jgoz opened a new pull request, #14380: React 18
jgoz opened a new pull request, #14380:
URL: https://github.com/apache/druid/pull/14380
### Description
Upgrades the React dependency to v18.
Note that there are several dependent packages whose peer dependency ranges still do not include 18. With the default npm peer dependency mode (strict), this would actually cause `npm install` to fail, so I switched to the legacy peer dependency mode. Keeping strict mode would involve upgrading, or in most cases, switching to alternate dependencies for those that have stopped receiving updates (react-table v6, react-splitter-layout, react-diff-viewer).
It was also not possible to switch to the [new JSX transform](https://legacy.reactjs.org/blog/2020/09/22/introducing-the-new-jsx-transform.html) because react-table has an implicit dependency on the `.createElement` API being used in order to distinguish between class and function components.
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [x] been self-reviewed.
- [x] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [x] been tested in a test Druid cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jgoz commented on a diff in pull request #14380: React 18
Posted by "jgoz (via GitHub)" <gi...@apache.org>.
jgoz commented on code in PR #14380:
URL: https://github.com/apache/druid/pull/14380#discussion_r1220508971
##########
web-console/src/singletons/toaster.ts:
##########
@@ -16,9 +16,16 @@
* limitations under the License.
*/
-import { Position, Toaster } from '@blueprintjs/core';
+import type { ToasterInstance } from '@blueprintjs/core';
+import { OverlayToaster, Position } from '@blueprintjs/core';
-export const AppToaster = Toaster.create({
- className: 'recipe-toaster',
- position: Position.TOP,
-});
+let toaster: ToasterInstance | undefined;
+
+export const AppToaster: { show: ToasterInstance['show'] } = {
+ show: (...args) => {
+ if (!toaster) {
+ toaster = OverlayToaster.create({ className: 'recipe-toaster', position: Position.TOP });
Review Comment:
Making this lazy-init avoids a deprecation notice for "ReactDOM.render", both on initial load and in every unit test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jgoz commented on a diff in pull request #14380: Upgrades the React dependency to v18
Posted by "jgoz (via GitHub)" <gi...@apache.org>.
jgoz commented on code in PR #14380:
URL: https://github.com/apache/druid/pull/14380#discussion_r1224482326
##########
web-console/package.json:
##########
@@ -170,5 +171,8 @@
"webpack-bundle-analyzer": "^4.4.1",
"webpack-cli": "^4.6.0",
"webpack-dev-server": "^3.11.2"
+ },
+ "overrides": {
+ "@types/react": "$@types/react"
Review Comment:
Some dependencies include `@types/react` as a dependency but pinned to specific versions. This forces those transitive dependency versions to resolve to the version specified in our package file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] vogievetsky commented on a diff in pull request #14380: Upgrades the React dependency to v18
Posted by "vogievetsky (via GitHub)" <gi...@apache.org>.
vogievetsky commented on code in PR #14380:
URL: https://github.com/apache/druid/pull/14380#discussion_r1223724499
##########
web-console/.npmrc:
##########
@@ -0,0 +1 @@
+legacy-peer-deps=true
Review Comment:
I think this file needs the Apache header to pass CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] vogievetsky merged pull request #14380: Upgrades the React dependency to v18
Posted by "vogievetsky (via GitHub)" <gi...@apache.org>.
vogievetsky merged PR #14380:
URL: https://github.com/apache/druid/pull/14380
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] vogievetsky commented on a diff in pull request #14380: Upgrades the React dependency to v18
Posted by "vogievetsky (via GitHub)" <gi...@apache.org>.
vogievetsky commented on code in PR #14380:
URL: https://github.com/apache/druid/pull/14380#discussion_r1223722343
##########
web-console/src/singletons/toaster.ts:
##########
@@ -16,9 +16,16 @@
* limitations under the License.
*/
-import { Position, Toaster } from '@blueprintjs/core';
+import type { ToasterInstance } from '@blueprintjs/core';
+import { OverlayToaster, Position } from '@blueprintjs/core';
-export const AppToaster = Toaster.create({
- className: 'recipe-toaster',
- position: Position.TOP,
-});
+let toaster: ToasterInstance | undefined;
+
+export const AppToaster: { show: ToasterInstance['show'] } = {
+ show: (...args) => {
+ if (!toaster) {
+ toaster = OverlayToaster.create({ className: 'recipe-toaster', position: Position.TOP });
Review Comment:
That is very insightful! Could you please add that comment (exactly as written) to the code itself?
Maybe also include that the warning is: `react-dom.development.js:86 Warning: ReactDOM.render is no longer supported in React 18. Use createRoot instead. Until you switch to the new API, your app will behave as if it's running React 17. Learn more: https://reactjs.org/link/switch-to-createroot`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] vogievetsky commented on a diff in pull request #14380: Upgrades the React dependency to v18
Posted by "vogievetsky (via GitHub)" <gi...@apache.org>.
vogievetsky commented on code in PR #14380:
URL: https://github.com/apache/druid/pull/14380#discussion_r1223722413
##########
web-console/package.json:
##########
@@ -170,5 +171,8 @@
"webpack-bundle-analyzer": "^4.4.1",
"webpack-cli": "^4.6.0",
"webpack-dev-server": "^3.11.2"
+ },
+ "overrides": {
+ "@types/react": "$@types/react"
Review Comment:
what does this do and why is it needed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org