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

Updated demos to React 18 #10300

Closed
wants to merge 33 commits into from
Closed

Updated demos to React 18 #10300

wants to merge 33 commits into from

Conversation

wszymanski
Copy link
Contributor

@wszymanski wszymanski commented Mar 22, 2023

Context

Done changes:

  • Updated docs/react-data-grid/demo/ and /react-data-grid/redux pages (they have been importing react-dom).
  • Updated examples project in the directory examples/next/docs/react.
  • Updated visual tests version of React to 18
  • Updated examples in https://github.com/handsontable/handsontable/tree/develop/examples/next/docs/react to React 18
  • Fixed problem with building examples (TypeScript error for subpackage in demo directory) See a comment below.
  • Updated JSFiddles (docs) to use React 18

I have been considering whether in handsontable/docs/.vuepress/public/scripts/fixer.js should be line:

} else if (key === 'react-dom/client' || key === 'react-dom') {

I've come to the conclusion that backward compatibility probably won't be needed due to rebuilding only the latest version of the documentation.

Note from the issue:

It should be checked on staging first - @aninde will test if all JSFiddles (docs) and examples work correctly and updated if everything works correctly.

Note: Some changes will be introduced to another repo: https://github.com/handsontable/handsontable.com-v2/pull/162

[skip changelog]

…ct 18 & Update examples in handsontable/handsontable@develop/examples/next/docs to React 18
@github-actions
Copy link

github-actions bot commented Mar 22, 2023

Launch the local version of documentation by running:

npm run docs:review db873d6efa7a2551d926eee5b3f1368b3f607ece

@wszymanski
Copy link
Contributor Author

wszymanski commented Mar 27, 2023

It seems that updating examples in develop/examples/next/docs/react to React 18 isn't possible without updating React wrapper dependencies to React 18. Otherwise, a TypeScript error is thrown:

Creating an optimized production build...
Failed to compile.

/home/runner/work/handsontable/handsontable/examples/next/docs/react/demo/src/index.tsx
TypeScript error in /home/runner/work/handsontable/handsontable/examples/next/docs/react/demo/src/index.tsx(22,6):
'HotTable' cannot be used as a JSX component.
  Its instance type 'HotTable' is not a valid JSX element.
    The types returned by 'render()' are incompatible between these types.
      Type 'ReactElement<any, string | JSXElementConstructor<any>>' is not assignable to type 'ReactNode'.
        Property 'children' is missing in type 'ReactElement<any, string | JSXElementConstructor<any>>' but required in type 'ReactPortal'.  TS2786

    20 | const App = () => {
    21 |   return (
  > 22 |     <HotTable
       |      ^
    23 |       data={data}
    24 |       height={4[50](https://github.com/handsontable/handsontable/actions/runs/4512356490/jobs/7945829667#step:21:51)}
    25 |       colWidths={[140, 192, 100, 90, 90, 110, 97, 100, 126]}

It can be reproduced and tested by building Handsontable, running npm run install:version next in handsontable/examples directory and building example by npm run build command within next/docs/react/demo directory.

There is problem with mismatching @types/react and @types/react-dom packages. After updating them to ^18.0.29 and ^18.0.11 (within 4450490 and 9bf0a21) it seems that problem hasn't been occurring.

It is possible to install dependencies in the single workspace or use npm dedupe in the example directory, but this workaround doesn't resolve the source of the problem. I suggest to update wrapper packages within another issue as it needs releasing of Handsontable. I reverted changes related to updating examples and visual tests within c4a14ea and 0a8612f so far.

@adamu-handsontable
Copy link

If I understand correctly, we have everything ready (using React 18) except examples (which are used mostly by us). Updating examples forces us to change React version in Handsontable React Wrapper to version 18. This could result in breaking changes for our users and seems like a separate issue. I think best way to move forward would be to merge what we already have and create a separate task for updating examples in the future.

@wszymanski wszymanski marked this pull request as ready for review June 12, 2023 16:19
@wszymanski
Copy link
Contributor Author

wszymanski commented Jun 12, 2023

@jansiegel This PR will release only selected features.

<App />,
document.getElementById('root')
);
const container = document.getElementById("container");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few things here:

  1. The syntax was changed to work with react@18, but react was not bumped to 18, so the examples don't work
  2. The targeted container id was changed to container from root here, but in the styles it's still root, so the example renders a blank page
  3. If updating the examples to react@18 requires a change in the wrapper's dependencies, it's highly possible that our wrapper is, at least in some instances, incompatible with react@18, right? Or am I missing something? And if that's the case, it should be probable looked into sooner than later. (cc @adamu-handsontable)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to 1 and 2 points. I don't know why it hasn't been reverted 🙄 Changed within 0a3d06e.

  1. I thought that problem is only related to our linking system and examples (part of the task), but it seems that there are some extra problems with ES build. Thus, I can agree that updating wrapper should be fixed sooner than later. Probably it will be a breaking change.

@wszymanski
Copy link
Contributor Author

@krzysztofspilka
Copy link
Member

As discussed on Aug 11, 2023, this issue needs further investigation. Not urgent.

@evanSe evanSe closed this May 15, 2024
@evanSe evanSe deleted the feature/dev-issue-1048 branch May 15, 2024 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants